| Message ID | 1433763511-5270-2-git-send-email-khandual@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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: "khandual@linux.vnet.ibm.com" <khandual@linux.vnet.ibm.com> > > '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 <khandual@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? > 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)) {
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@linux.vnet.ibm.com" <khandual@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@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.
> >> > >> - 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. > What I meant was, should this read (cpuhw->bhrb_users != 0)? Because bhrb_users in check_excludes() is a signed int, I also wanted to make sure it shouldn't be a test for bhrb_users > 0 instead. (Also, if bhrb_users is always positive, should it be an unsigned int?) I don't think a nested conditional would be better. > >> - 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. > I see that, but I still think you should take this opportunity to improve it. Regards, Daniel
On 06/11/2015 08:58 AM, Daniel Axtens wrote: > >>>> >>>> - 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. >> > > What I meant was, should this read (cpuhw->bhrb_users != 0)? Because > bhrb_users in check_excludes() is a signed int, I also wanted to make > sure it shouldn't be a test for bhrb_users > 0 instead. (Also, if > bhrb_users is always positive, should it be an unsigned int?) Will replace both the conditional checks in comparison against 0. Will change the data type of bhrb_users into unsigned int as well. > > I don't think a nested conditional would be better. Okay. > > > >>>> - 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. >> > I see that, but I still think you should take this opportunity to > improve it. Will try to change it in a separate patch.
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. */ 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. */ - 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) { /* * 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); + if (check_excludes(ctrs, cflags, n, 1, cpuhw->bhrb_users)) { + put_cpu_var(cpu_hw_events); return -EINVAL; + } - cpuhw = &get_cpu_var(cpu_hw_events); err = power_check_constraints(cpuhw, events, cflags, n + 1); if (has_branch_stack(event)) {