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: 1534 bytes --]


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

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