Re: [PATCH] Remove Powerpc OProfile events the kernel will reject
William Cohen <[email protected]>
| Newsgroups | gmane.linux.oprofile |
|---|---|
| Message-ID | <[email protected]> |
On 11/16/2015 03:48 PM, Carl E. Love wrote:
> On Mon, 2015-11-16 at 14:46 -0500, William Cohen wrote:
>> On 11/13/2015 03:27 PM, Carl E. Love wrote:
>>>
>>>
>>> Will:
>>>
>>> I have created a patch to remove the Power 8 events that the perf kernel code will reject. This is a patch
>>> to address the issue of the PM_L2_LD_MISS event not working as reported by Chencheng Ye on November 5.
>>>
>>> I don't know why the design had the setup register only writable by the Hypervisor. I also checked and as far
>>> as I could find out there isn't any work being done at the moment to create the needed API to allow the kernel
>>> to request the Hypervisor write the needed bits into the configuration register. So, we have no option at the
>>> moment but to remove the events.
>>>
>>> I actually just commented them out with a detailed explanation in the hope that we will be able to re-enable them
>>> in the future. Please let me know if you have any feedback on the patch. Thanks for your time.
>>>
>>> Carl Love
>>
>> Hi Carl,
>>
>> I took a look at this patch today. How was the list of events to disabled determined? Looking at the power8-pmu.c file is the following correct to find the events setting the caching caching bit (look at events that have unit 6,7,8, or 9 and check for non-zero ?
>>
>> egrep "event:0x[1-9a-fA-F][0-9a-fA-F][6789][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F] " events/ppc64/power8/events |more
>>
>> Are the following with unit=9 okay? They don't seem to be comments out by the patch:
>>
>> $ egrep "event:0x[1-9a-fA-F][0-9a-fA-F][6789][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F] " events/ppc64/power8/events |grep -v "##"
>> event:0x819082 counters:0 um:zero minimum:10000 name:PM_L3_CI_USAGE : rotating sample of 16 CI or CO actives
>> event:0x83908b counters:2 um:zero minimum:10000 name:PM_L3_CO0_ALLOC : 0.0
>> event:0x83908a counters:2 um:zero minimum:10000 name:PM_L3_CO0_BUSY : lifetime, sample of CO machine 0 valid
>> event:0x84908d counters:3 um:zero minimum:10000 name:PM_L3_PF0_ALLOC : 0.0
>> event:0x84908c counters:3 um:zero minimum:10000 name:PM_L3_PF0_BUSY : lifetime, sample of PF machine 0 valid
>> event:0x829084 counters:1 um:zero minimum:10000 name:PM_L3_PF_USAGE : rotating sample of 32 PF actives
>> event:0x84908f counters:3 um:zero minimum:10000 name:PM_L3_RD0_ALLOC : 0.0
>> event:0x84908e counters:3 um:zero minimum:10000 name:PM_L3_RD0_BUSY : lifetime, sample of RD machine 0 valid
>> event:0x829086 counters:1 um:zero minimum:10000 name:PM_L3_RD_USAGE : rotating sample of 16 RD actives
>> event:0x839089 counters:2 um:zero minimum:10000 name:PM_L3_SN0_ALLOC : 0.0
>> event:0x839088 counters:2 um:zero minimum:10000 name:PM_L3_SN0_BUSY : lifetime, sample of snooper machine 0 valid
>> event:0x819080 counters:0 um:zero minimum:10000 name:PM_L3_SN_USAGE : rotating sample of 8 snoop valids
>>
>>
>> -Will
>
> Will and Michael:
>
> If you look at a recent kernel has the following code in
> linux-4.3/arch/powerpc/perf/power8-pmu.c
>
>
>
> at line 284:
>
> static int power8_get_constraint(u64 event, unsigned long *maskp, unsigned long\
> *valp)
> {
> unsigned int unit, pmc, cache, ebb;
> unsigned long mask, value;
>
> mask = value = 0;
>
> if (event & ~EVENT_VALID_MASK)
> return -1;
>
> pmc = (event >> EVENT_PMC_SHIFT) & EVENT_PMC_MASK;
> unit = (event >> EVENT_UNIT_SHIFT) & EVENT_UNIT_MASK;
> cache = (event >> EVENT_CACHE_SEL_SHIFT) & EVENT_CACHE_SEL_MASK;
> ebb = (event >> EVENT_EBB_SHIFT) & EVENT_EBB_MASK;
>
> if (pmc) {
>
> The pmc, the unit for the event and the cache setting bits are extracted from the event. The cache
> are bits [23:20] of the event, the unit is bits [15:12] of the event as described in the comment at
> line 74.
>
> Then at about line 326 we have the code:
>
> if (unit >= 6 && unit <= 9) {
> /*
> * L2/L3 events contain a cache selector field, which is
> * supposed to be programmed into MMCRC. However MMCRC is only
> * HV writable, and there is no API for guest kernels to modify
> * it. The solution is for the hypervisor to initialise the
> * field to zeroes, and for us to only ever allow events that
> * have a cache selector of zero. The bank selector (bit 3) is
> * irrelevant, as long as the rest of the value is 0.
> */
> if (cache & 0x7)
> return -1;
>
> Which says any event having unit 6, 7, 8 or 9 with the lower three bits of the cache bits that
> are not zero will be rejected. Unfortunately, there is no check for the kernel running as the
> hypervisor. So, it doesn't matter what hypervisor is in use or if the system is not virtualized,
> the kernel will reject the events.
>
> So, the events you mentioned Will, are not rejected. The comment seems to indicate that the bank
> selector bit, the 8, in the events you mentioned is irrelevant.
>
> Carl Love
>
Hi Carl,
So if I am reading the comment around line 326 the code is allowed to have the msb of cache_sel be 1 so long as the lower 3 bits are zero. Thus, 8 and 0 are allowed in for the cache_sel nibble and the currently proposed patch does exclude the problem pmu events.
The libpfm4/lib/events/power8.h has:
* Documentation on the PMU events will be published at:
* http://www.power.org/documentation
Is there a updated url to point people at for the power8 PMU documentation?
-Will
------------------------------------------------------------------------------
Presto, an open source distributed SQL query engine for big data, initially
developed by Facebook, enables you to easily query your data on Hadoop in a
more interactive manner. Teradata is also now providing full enterprise
support for Presto. Download a free open source copy now.
http://pubads.g.doubleclick.net/gampad/clk?id=250295911&iu=/4140