Re: [PATCH V4 08/10] powerpc, perf: Enable SW filtering in branch stack sampling framework

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

On 12/10/2013 11:27 AM, Anshuman Khandual wrote:
> On 12/09/2013 11:51 AM, Michael Ellerman wrote:
>> This code was already in need of some unindentation, and now it's just
>> ridiculous.
>>
>> To start with at the beginning of this routine we have:
>>
>> while (..) {
>> 	if (!val)
>> 		break;
>> 	else {
>> 		// Bulk of the logic
>> 		...
>> 	}
>> }
>>
>> That should almost always become:
>>
>> while (..) {
>> 	if (!val)
>> 		break;
>>
>> 	// Bulk of the logic
>> 	...
>> }
>>
>>
>> But in this case that's not enough. Please send a precursor patch which moves
>> this logic out into a helper function.
> 
> Hey Michael,
> 
> I believe this patch should be able to take care of this.
> 
> commit d66d729715cabe0cfd8e34861a6afa8ad639ddf3
> Author: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> Date:   Tue Dec 10 11:10:06 2013 +0530
> 
>     power, perf: Clean up BHRB processing
>     
>     This patch cleans up some indentation problem and re-organizes the
>     BHRB processing code with an additional helper function.
>     
>     Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> 
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 29b89e8..9ae96c5 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -400,11 +400,20 @@ static __u64 power_pmu_bhrb_to(u64 addr)
>  	return target - (unsigned long)&instr + addr;
>  }
> 
> +void update_branch_entry(struct cpu_hw_events *cpuhw, int u_index, u64 from, u64 to, int pred)
> +{
> +	cpuhw->bhrb_entries[u_index].from = from;
> +	cpuhw->bhrb_entries[u_index].to = to;
> +	cpuhw->bhrb_entries[u_index].mispred = pred;
> +	cpuhw->bhrb_entries[u_index].predicted = ~pred;
> +	return;
> +}
> +
>  /* Processing BHRB entries */
>  void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
>  {
>  	u64 val;
> -	u64 addr;
> +	u64 addr, tmp;
>  	int r_index, u_index, pred;
> 
>  	r_index = 0;
> @@ -415,62 +424,54 @@ void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
>  		if (!val)
>  			/* Terminal marker: End of valid BHRB entries */
>  			break;
> -		else {
> -			addr = val & BHRB_EA;
> -			pred = val & BHRB_PREDICTION;
> 
> -			if (!addr)
> -				/* invalid entry */
> -				continue;
> +		addr = val & BHRB_EA;
> +		pred = val & BHRB_PREDICTION;
> 
> -			/* Branches are read most recent first (ie. mfbhrb 0 is
> -			 * the most recent branch).
> -			 * There are two types of valid entries:
> -			 * 1) a target entry which is the to address of a
> -			 *    computed goto like a blr,bctr,btar.  The next
> -			 *    entry read from the bhrb will be branch
> -			 *    corresponding to this target (ie. the actual
> -			 *    blr/bctr/btar instruction).
> -			 * 2) a from address which is an actual branch.  If a
> -			 *    target entry proceeds this, then this is the
> -			 *    matching branch for that target.  If this is not
> -			 *    following a target entry, then this is a branch
> -			 *    where the target is given as an immediate field
> -			 *    in the instruction (ie. an i or b form branch).
> -			 *    In this case we need to read the instruction from
> -			 *    memory to determine the target/to address.
> +		if (!addr)
> +			/* invalid entry */
> +			continue;
> +
> +		/* Branches are read most recent first (ie. mfbhrb 0 is
> +		 * the most recent branch).
> +		 * There are two types of valid entries:
> +		 * 1) a target entry which is the to address of a
> +		 *    computed goto like a blr,bctr,btar.  The next
> +		 *    entry read from the bhrb will be branch
> +		 *    corresponding to this target (ie. the actual
> +		 *    blr/bctr/btar instruction).
> +		 * 2) a from address which is an actual branch.  If a
> +		 *    target entry proceeds this, then this is the
> +		 *    matching branch for that target.  If this is not
> +		 *    following a target entry, then this is a branch
> +		 *    where the target is given as an immediate field
> +		 *    in the instruction (ie. an i or b form branch).
> +		 *    In this case we need to read the instruction from
> +		 *    memory to determine the target/to address.
> +		 */
> +		if (val & BHRB_TARGET) {
> +			/* Target branches use two entries
> +			 * (ie. computed gotos/XL form)
>  			 */
> +			tmp = addr;
> 
> +			/* Get from address in next entry */
> +			val = read_bhrb(r_index++);
> +			addr = val & BHRB_EA;
>  			if (val & BHRB_TARGET) {
> -				/* Target branches use two entries
> -				 * (ie. computed gotos/XL form)
> -				 */
> -				cpuhw->bhrb_entries[u_index].to = addr;
> -				cpuhw->bhrb_entries[u_index].mispred = pred;
> -				cpuhw->bhrb_entries[u_index].predicted = ~pred;
> -
> -				/* Get from address in next entry */
> -				val = read_bhrb(r_index++);
> -				addr = val & BHRB_EA;
> -				if (val & BHRB_TARGET) {
> -					/* Shouldn't have two targets in a
> -					   row.. Reset index and try again */
> -					r_index--;
> -					addr = 0;
> -				}
> -				cpuhw->bhrb_entries[u_index].from = addr;
> -			} else {
> -				/* Branches to immediate field 
> -				   (ie I or B form) */
> -				cpuhw->bhrb_entries[u_index].from = addr;
> -				cpuhw->bhrb_entries[u_index].to =
> -					power_pmu_bhrb_to(addr);
> -				cpuhw->bhrb_entries[u_index].mispred = pred;
> -				cpuhw->bhrb_entries[u_index].predicted = ~pred;
> +				/* Shouldn't have two targets in a
> +				   row.. Reset index and try again */
> +				r_index--;
> +				addr = 0;
>  			}
> -			u_index++;
> -
> +			update_branch_entry(cpuhw, u_index, addr, tmp, pred);
> +		} else {
> +			/* Branches to immediate field 
> +			   (ie I or B form) */
> +			tmp = power_pmu_bhrb_to(addr);
> +			update_branch_entry(cpuhw, u_index, addr, tmp, pred);
>  		}
> +		u_index++;
>  	}
>  	cpuhw->bhrb_stack.nr = u_index;
>  	return;

Hey Michael,

Does the patch looks okay ? In which case will send it out separately. Do let
me know. Thank you.

Regards
Anshuman