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

[Anshuman Khandual <khandual@linux.vnet.ibm.com>]

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.