[PATCH V8 02/10] powerpc, perf: Restore privillege level filter support for BHRB
Anshuman Khandual
khandual at linux.vnet.ibm.com
Wed Jun 10 22:08:43 AEST 2015
On 06/10/2015 09:13 AM, Daniel Axtens wrote:
> In the subject line, privilege should only have 1 l, and I think it
> should probably start with "powerpc/perf:" rather than "powerpc, perf:".
Will fix the typo here. Have been using "powerpc, perf:" format for some
time now :) Seems to be more cleaner compared to "powerpc/perf:" format.
But again its subjective.
> > On Mon, 2015-06-08 at 17:08 +0530, Anshuman Khandual wrote:
>> From: "khandual at linux.vnet.ibm.com" <khandual at linux.vnet.ibm.com>
>>
>> 'commit 9de5cb0f6df8 ("powerpc/perf: Add per-event excludes on Power8")'
> Does this need a 'Fixes:' tag then?
Not really, it only fixes the BHRB privilege request cases not other
scenarios which are impacted by this previous commit.
>
>> broke the PMU based BHRB privilege level filter. BHRB depends on the
>> same MMCR0 bits for privilege level filter which was used to freeze all
>> the PMCs as a group. Once we moved to individual event based privilege
>> filters through MMCR2 register on POWER8, event associated privilege
>> filters are no longer applicable to the BHRB captured branches.
>>
>> This patch solves the problem by restoring to the previous method of
>> privilege level filters for the event in case BHRB based branch stack
>> sampling is requested. This patch also changes 'check_excludes' for
>> the same reason.
>>
>> Signed-off-by: Anshuman Khandual <khandual at linux.vnet.ibm.com>
>> ---
>> arch/powerpc/perf/core-book3s.c | 19 +++++++++++--------
>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index c246e65..ae61629 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -930,7 +930,7 @@ static int power_check_constraints(struct cpu_hw_events *cpuhw,
>> * added events.
>> */
> Does this comment need to be updated?
Not really. The previous commit did not update it, hence this patch would
skip it as well.
>> static int check_excludes(struct perf_event **ctrs, unsigned int cflags[],
>> - int n_prev, int n_new)
>> + int n_prev, int n_new, int bhrb_users)
>> {
>> int eu = 0, ek = 0, eh = 0;
>> int i, n, first;
>> @@ -941,7 +941,7 @@ static int check_excludes(struct perf_event **ctrs, unsigned int cflags[],
>> * don't need to do any of this logic. NB. This assumes no PMU has both
>> * per event exclude and limited PMCs.
>> */
> Likewise, does this comment need to be updated?
Yeah, will update it.
>> - if (ppmu->flags & PPMU_ARCH_207S)
>> + if ((ppmu->flags & PPMU_ARCH_207S) && !bhrb_users)
>> return 0;
>>
>> n = n_prev + n_new;
>> @@ -1259,7 +1259,7 @@ static void power_pmu_enable(struct pmu *pmu)
>> goto out;
>> }
>>
>> - if (!(ppmu->flags & PPMU_ARCH_207S)) {
>> + if (!(ppmu->flags & PPMU_ARCH_207S) || cpuhw->bhrb_users)
> You're using cpuhw->bhrb_users as a bool here, where it's an int. Could
> you make the test more specific so that it's clear exactly what you're
> expecting bhrb_users to contain?
Using cpuhw->bhrb_users as a bool just verifies whether it contains
zero or non-zero value in it. The test seems to be doing that as
expected. But yes, we can move it as a nested conditional block as
well if that is better.
>> {
>> /*
>> * Add in MMCR0 freeze bits corresponding to the attr.exclude_*
>> * bits for the first event. We have already checked that all
>> @@ -1284,7 +1284,7 @@ static void power_pmu_enable(struct pmu *pmu)
>> mtspr(SPRN_MMCR1, cpuhw->mmcr[1]);
>> mtspr(SPRN_MMCR0, (cpuhw->mmcr[0] & ~(MMCR0_PMC1CE | MMCR0_PMCjCE))
>> | MMCR0_FC);
>> - if (ppmu->flags & PPMU_ARCH_207S)
>> + if ((ppmu->flags & PPMU_ARCH_207S) && !cpuhw->bhrb_users)
>> mtspr(SPRN_MMCR2, cpuhw->mmcr[3]);
>>
>> /*
>> @@ -1436,7 +1436,8 @@ static int power_pmu_add(struct perf_event *event, int ef_flags)
>> if (cpuhw->group_flag & PERF_EVENT_TXN)
>> goto nocheck;
>>
>> - if (check_excludes(cpuhw->event, cpuhw->flags, n0, 1))
>> + if (check_excludes(cpuhw->event, cpuhw->flags,
>> + n0, 1, cpuhw->bhrb_users))
>> goto out;
>> if (power_check_constraints(cpuhw, cpuhw->events, cpuhw->flags, n0 + 1))
>> goto out;
>> @@ -1615,7 +1616,7 @@ static int power_pmu_commit_txn(struct pmu *pmu)
>> return -EAGAIN;
>> cpuhw = this_cpu_ptr(&cpu_hw_events);
>> n = cpuhw->n_events;
>> - if (check_excludes(cpuhw->event, cpuhw->flags, 0, n))
>> + if (check_excludes(cpuhw->event, cpuhw->flags, 0, n, cpuhw->bhrb_users))
>> return -EAGAIN;
>> i = power_check_constraints(cpuhw, cpuhw->events, cpuhw->flags, n);
>> if (i < 0)
>> @@ -1828,10 +1829,12 @@ static int power_pmu_event_init(struct perf_event *event)
>> events[n] = ev;
>> ctrs[n] = event;
>> cflags[n] = flags;
>> - if (check_excludes(ctrs, cflags, n, 1))
>> + cpuhw = &get_cpu_var(cpu_hw_events);
> Should this be using a this_cpu_ptr rather than a get_cpu_var? (as with
> the power_pmu_commit_txn case?)
>> + if (check_excludes(ctrs, cflags, n, 1, cpuhw->bhrb_users)) {
>> + put_cpu_var(cpu_hw_events);
> Likewise with this?
>> return -EINVAL;
>> + }
>>
>> - cpuhw = &get_cpu_var(cpu_hw_events);
This patch just moves the existing code couple of lines above without
changing it in any manner.
More information about the Linuxppc-dev
mailing list