Re: [PATCH V8 02/10] powerpc, perf: Restore privillege level filter support for BHRB

[Daniel Axtens <dja@axtens.net>]

[-- Attachment #1: Type: text/plain, Size: 4559 bytes --]

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)) {


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 860 bytes --]