In the subject line, privilege should only have 1 l, and I think it should probably start with "powerpc/perf:" rather than "powerpc, perf:".
On Mon, 2015-06-08 at 17:08 +0530, Anshuman Khandual wrote: > From: "[email protected]" <[email protected]> > > 'commit 9de5cb0f6df8 ("powerpc/perf: Add per-event excludes on Power8")' Does this need a 'Fixes:' tag then? > 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 <[email protected]> > --- > 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? > 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? > - 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? > { > /* > * 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); > err = power_check_constraints(cpuhw, events, cflags, n + 1); > > if (has_branch_stack(event)) {
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Linuxppc-dev mailing list [email protected] https://lists.ozlabs.org/listinfo/linuxppc-dev
