Patchwork [V3,02/10] powerpc, perf: Enable conditional branch filter for POWER8

login
register
mail settings
Submitter Anshuman Khandual
Date Oct. 16, 2013, 6:56 a.m.
Message ID <1381906617-11392-3-git-send-email-khandual@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/283856/
State Superseded, archived
Delegated to: Michael Ellerman
Headers show

Comments

Anshuman Khandual - Oct. 16, 2013, 6:56 a.m.
Enables conditional branch filter support for POWER8
utilizing MMCRA register based filter and also invalidates
a BHRB branch filter combination involving conditional
branches.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/perf/power8-pmu.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
Michael Ellerman - Nov. 26, 2013, 6:06 a.m.
On Wed, 2013-16-10 at 06:56:49 UTC, Anshuman Khandual wrote:
> Enables conditional branch filter support for POWER8
> utilizing MMCRA register based filter and also invalidates
> a BHRB branch filter combination involving conditional
> branches.
> 
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> ---
>  arch/powerpc/perf/power8-pmu.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
> index 2ee4a70..6e28587 100644
> --- a/arch/powerpc/perf/power8-pmu.c
> +++ b/arch/powerpc/perf/power8-pmu.c
> @@ -580,11 +580,21 @@ static u64 power8_bhrb_filter_map(u64 branch_sample_type)
>  	if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL)
>  		return -1;
>  
> +	/* Invalid branch filter combination - HW does not support */
> +	if ((branch_sample_type & PERF_SAMPLE_BRANCH_ANY_CALL) &&
> +			(branch_sample_type & PERF_SAMPLE_BRANCH_COND))
> +		return -1;

What this doesn't make obvious is that the hardware doesn't support any
combinations. It just happens that these are the only two possibilities we
allow, and so this is the only combination we have to disallow.

>
>  	if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_CALL) {
>  		pmu_bhrb_filter |= POWER8_MMCRA_IFM1;
>  		return pmu_bhrb_filter;
>  	}
>  
> +	if (branch_sample_type & PERF_SAMPLE_BRANCH_COND) {
> +		pmu_bhrb_filter |= POWER8_MMCRA_IFM3;
> +		return pmu_bhrb_filter;
> +	}
> +
>  	/* Every thing else is unsupported */
>  	return -1;
>  }

I think it would be clearer if we actually checked for the possibilities we
allow and let everything else fall through, eg:

	/* Ignore user/kernel/hv bits */
	branch_sample_type &= ~PERF_SAMPLE_BRANCH_PLM_ALL;

	if (branch_sample_type == PERF_SAMPLE_BRANCH_ANY)
		return 0;

	if (branch_sample_type == PERF_SAMPLE_BRANCH_ANY_CALL)
		return POWER8_MMCRA_IFM1;
 
	if (branch_sample_type == PERF_SAMPLE_BRANCH_COND)
		return POWER8_MMCRA_IFM3;
	
	return -1;

cheers
Anshuman Khandual - Nov. 26, 2013, 10:40 a.m.
On 11/26/2013 11:36 AM, mpe@ellerman.id.au wrote:
> On Wed, 2013-16-10 at 06:56:49 UTC, Anshuman Khandual wrote:
>> Enables conditional branch filter support for POWER8
>> utilizing MMCRA register based filter and also invalidates
>> a BHRB branch filter combination involving conditional
>> branches.
>>
>> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/perf/power8-pmu.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
>> index 2ee4a70..6e28587 100644
>> --- a/arch/powerpc/perf/power8-pmu.c
>> +++ b/arch/powerpc/perf/power8-pmu.c
>> @@ -580,11 +580,21 @@ static u64 power8_bhrb_filter_map(u64 branch_sample_type)
>>  	if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL)
>>  		return -1;
>>  
>> +	/* Invalid branch filter combination - HW does not support */
>> +	if ((branch_sample_type & PERF_SAMPLE_BRANCH_ANY_CALL) &&
>> +			(branch_sample_type & PERF_SAMPLE_BRANCH_COND))
>> +		return -1;
> 
> What this doesn't make obvious is that the hardware doesn't support any
> combinations. It just happens that these are the only two possibilities we
> allow, and so this is the only combination we have to disallow.
> 
>>
>>  	if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_CALL) {
>>  		pmu_bhrb_filter |= POWER8_MMCRA_IFM1;
>>  		return pmu_bhrb_filter;
>>  	}
>>  
>> +	if (branch_sample_type & PERF_SAMPLE_BRANCH_COND) {
>> +		pmu_bhrb_filter |= POWER8_MMCRA_IFM3;
>> +		return pmu_bhrb_filter;
>> +	}
>> +
>>  	/* Every thing else is unsupported */
>>  	return -1;
>>  }
> 
> I think it would be clearer if we actually checked for the possibilities we
> allow and let everything else fall through, eg:
> 
> 	/* Ignore user/kernel/hv bits */
> 	branch_sample_type &= ~PERF_SAMPLE_BRANCH_PLM_ALL;
> 
> 	if (branch_sample_type == PERF_SAMPLE_BRANCH_ANY)
> 		return 0;
> 
> 	if (branch_sample_type == PERF_SAMPLE_BRANCH_ANY_CALL)
> 		return POWER8_MMCRA_IFM1;
> 
> 	if (branch_sample_type == PERF_SAMPLE_BRANCH_COND)
> 		return POWER8_MMCRA_IFM3;
> 	
> 	return -1;
> 

Please look at the 9th patch (power8, perf: Change BHRB branch filter configuration).
All these issues are taken care of in this patch. It clearly indicates that any combination
of HW BHRB filters will not be supported in the PMU and hence zero out the HW filter component
and processes all of those filters in the SW.

Patch

diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
index 2ee4a70..6e28587 100644
--- a/arch/powerpc/perf/power8-pmu.c
+++ b/arch/powerpc/perf/power8-pmu.c
@@ -580,11 +580,21 @@  static u64 power8_bhrb_filter_map(u64 branch_sample_type)
 	if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL)
 		return -1;
 
+	/* Invalid branch filter combination - HW does not support */
+	if ((branch_sample_type & PERF_SAMPLE_BRANCH_ANY_CALL) &&
+			(branch_sample_type & PERF_SAMPLE_BRANCH_COND))
+		return -1;
+
 	if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_CALL) {
 		pmu_bhrb_filter |= POWER8_MMCRA_IFM1;
 		return pmu_bhrb_filter;
 	}
 
+	if (branch_sample_type & PERF_SAMPLE_BRANCH_COND) {
+		pmu_bhrb_filter |= POWER8_MMCRA_IFM3;
+		return pmu_bhrb_filter;
+	}
+
 	/* Every thing else is unsupported */
 	return -1;
 }