Re: [PATCH V8 03/10] powerpc, perf: Re organize BHRB processing

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

On 06/10/2015 10:06 AM, Daniel Axtens wrote:
> 
>> +void update_branch_entry(struct cpu_hw_events *cpuhw,
>> +			int index, u64 from, u64 to, int pred)
>> +{
>> +	cpuhw->bhrb_entries[index].from = from;
>> +	cpuhw->bhrb_entries[index].to = to;
>> +	cpuhw->bhrb_entries[index].mispred = pred;
>> +	cpuhw->bhrb_entries[index].predicted = ~pred;
>> +}
> 
> I realise you're copying existing code, but:
>  - could you please rename pred? If we assign .mispred to pred
> and .predicted to ~pred, we should pick a different name for pred.

Agreed.

>  - I'm really uncomfortable with the bitwise inverting a signed integer.
> Can you explain what is going on here? Looking at
> include/uapi/linux/perf_event.h, this seems to be a single bit flag:
> shouldn't this then be a logical flip rather than a bitwise one?
> (Furthermore, looking at that header, why is pred an int at all? Why not
> a bool?)

Agreed.

> 
>> +
>>  /* Processing BHRB entries */
>>  static void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
>>  {
>> -	u64 val;
>> -	u64 addr;
>> +	u64 val, addr, tmp;
> Please don't use 'tmp' here. As far as I can tell, you use this variable
> to compute the 'to' address. The name should reflect that.

Agreed but then it will be a new preparatory patch at the beginning
of this patch series.