[PATCH 0/2] Improvement and fixes for BHRB

[PATCH 0/2] Improvement and fixes for BHRB

[Anshuman Khandual]

(1) The first patch fixes a situation like this

Before patch:-
------------

./perf record -j any -e branch-misses:k ls
Error:
The sys_perf_event_open() syscall returned with 95 (Operation not supported) for event (branch-misses:k).
/bin/dmesg may provide additional information.
No CONFIG_PERF_EVENTS=y kernel support configured?

Here 'perf record' actually copies over ':k' filter request into BHRB
privilege state filter config and our previous check in kernel would
fail that.

After patch:-
-------------

/perf record -j any -e branch-misses:k ls
perf  perf.data  perf.data.old  test-mmap-ring
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.002 MB perf.data (~102 samples) ]

(2) The second patch fixes context migration for BHRB filter configuration

Anshuman Khandual (2):
  powerpc, perf: Ignore separate BHRB privilege state filter request
  powerpc, perf: BHRB filter configuration should follow the task

 arch/powerpc/perf/core-book3s.c |  5 ++++-
 arch/powerpc/perf/power8-pmu.c  | 17 +++++++++--------
 2 files changed, 13 insertions(+), 9 deletions(-)

-- 
1.7.11.7

[PATCH 1/2] powerpc, perf: Ignore separate BHRB privilege state filter request

[Anshuman Khandual]

Completely ignore BHRB privilege state filter request as we are
already configuring MMCRA register with privilege state filtering
attribute for the accompanying PMU event. This would help achieve
cleaner user space interaction for BHRB.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/perf/power8-pmu.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
index f7d1c4f..8ed323d 100644
--- a/arch/powerpc/perf/power8-pmu.c
+++ b/arch/powerpc/perf/power8-pmu.c
@@ -525,16 +525,17 @@ static u64 power8_bhrb_filter_map(u64 branch_sample_type)
 	u64 pmu_bhrb_filter = 0;
 	u64 br_privilege = branch_sample_type & ONLY_PLM;
 
-	/* BHRB and regular PMU events share the same prvillege state
+	/* BHRB and regular PMU events share the same prvilege state
 	 * filter configuration. BHRB is always recorded along with a
-	 * regular PMU event. So privilege state filter criteria for BHRB
-	 * and the companion PMU events has to be the same. As a default
-	 * "perf record" tool sets all privillege bits ON when no filter
-	 * criteria is provided in the command line. So as along as all
-	 * privillege bits are ON or they are OFF, we are good to go.
+	 * regular PMU event. So privilege state filter criteria for
+	 * the BHRB and the companion PMU events has to be the same.
+	 * Separate BHRB privillege state filter requests would be
+	 * ignored.
 	 */
-	if ((br_privilege != 7) && (br_privilege != 0))
-		return -1;
+
+	if (br_privilege)
+		pr_info("BHRB privilege state filter request %llx ignored\n",
+								br_privilege);
 
 	/* No branch filter requested */
 	if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY)
-- 
1.7.11.7

Re: [PATCH 1/2] powerpc, perf: Ignore separate BHRB privilege state filter request

[Michael Ellerman]

On Wed, 2013-05-22 at 11:17 +0530, Anshuman Khandual wrote:
> Completely ignore BHRB privilege state filter request as we are
> already configuring MMCRA register with privilege state filtering
> attribute for the accompanying PMU event. This would help achieve
> cleaner user space interaction for BHRB.

Your description from patch 0 should be here.


> diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
> index f7d1c4f..8ed323d 100644
> --- a/arch/powerpc/perf/power8-pmu.c
> +++ b/arch/powerpc/perf/power8-pmu.c
> @@ -525,16 +525,17 @@ static u64 power8_bhrb_filter_map(u64 branch_sample_type)
>  	u64 pmu_bhrb_filter = 0;
>  	u64 br_privilege = branch_sample_type & ONLY_PLM;
>  
> -	/* BHRB and regular PMU events share the same prvillege state
> +	/* BHRB and regular PMU events share the same prvilege state

Please spell "privilege" correctly.

>  	 * filter configuration. BHRB is always recorded along with a
> -	 * regular PMU event. So privilege state filter criteria for BHRB
> -	 * and the companion PMU events has to be the same. As a default
> -	 * "perf record" tool sets all privillege bits ON when no filter
> -	 * criteria is provided in the command line. So as along as all
> -	 * privillege bits are ON or they are OFF, we are good to go.
> +	 * regular PMU event. So privilege state filter criteria for
> +	 * the BHRB and the companion PMU events has to be the same.
> +	 * Separate BHRB privillege state filter requests would be
> +	 * ignored.
>  	 */

This comment doesn't make sense to me with the updated code.

It still says "privilege state filter criteria for the BHRB and the
companion PMU events has to be the same".

But they don't, right?

What it should say is "we ignore the privilege bits in the branch sample
type because they are handled by the underlying PMC configuration" - or
something like that.

> -	if ((br_privilege != 7) && (br_privilege != 0))
> -		return -1;
> +
> +	if (br_privilege)
> +		pr_info("BHRB privilege state filter request %llx ignored\n",
> +								br_privilege);

Don't do that. Ignoring the br_privilege is either the right thing to do
in which case we do it and print nothing, or it doesn't make sense and
we reject it.

cheers

Re: [PATCH 1/2] powerpc, perf: Ignore separate BHRB privilege state filter request

[Anshuman Khandual]

> 
> Your description from patch 0 should be here.
>

Sure, will bring it here.

> 
>> diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
>> index f7d1c4f..8ed323d 100644
>> --- a/arch/powerpc/perf/power8-pmu.c
>> +++ b/arch/powerpc/perf/power8-pmu.c
>> @@ -525,16 +525,17 @@ static u64 power8_bhrb_filter_map(u64 branch_sample_type)
>>  	u64 pmu_bhrb_filter = 0;
>>  	u64 br_privilege = branch_sample_type & ONLY_PLM;
>>  
>> -	/* BHRB and regular PMU events share the same prvillege state
>> +	/* BHRB and regular PMU events share the same prvilege state
> 
> Please spell "privilege" correctly.
> 

My bad, will fix it.


>>  	 * filter configuration. BHRB is always recorded along with 
> It still says "privilege state filter criteria for the BHRB and the
> companion PMU events has to be the same".
> 
> But they don't, right?
> 

Right


> What it should say is "we ignore the privilege bits in the branch sample
> type because they are handled by the underlying PMC configuration" - or
> something like that.

Here is the latest description for the code block

	/* BHRB and regular PMU events share the same privilege state
	 * filter configuration. BHRB is always recorded along with a
	 * regular PMU event. As the privilege state filter is handled
	 * in the basic PMC configuration of the accompanying regular
	 * PMU event, we ignore any separate BHRB specific request.
	 */

Does it sound better ?

> 
>> -	if ((br_privilege != 7) && (br_privilege != 0))
>> -		return -1;
>> +
>> +	if (br_privilege)
>> +		pr_info("BHRB privilege state filter request %llx ignored\n",
>> +								br_privilege);
> 
> Don't do that. Ignoring the br_privilege is either the right thing to do
> in which case we do it and print nothing,


I thought the informational print would at least make the user aware
of the fact that the separate filter request for BHRB went ignored.
Can we add this some where in the documentation ?

  
 or it doesn't make sense and
> we reject it.
>

 
> cheers
> 

Re: [PATCH 1/2] powerpc, perf: Ignore separate BHRB privilege state filter request

[Anshuman Khandual]

On 05/22/2013 02:29 PM, Anshuman Khandual wrote:
>>
>> Your description from patch 0 should be here.
> Does it sound better ?
> 
>>
>>> -	if ((br_privilege != 7) && (br_privilege != 0))
>>> -		return -1;
>>> +
>>> +	if (br_privilege)
>>> +		pr_info("BHRB privilege state filter request %llx ignored\n",
>>> +								br_privilege);
>>
>> Don't do that. Ignoring the br_privilege is either the right thing to do
>> in which case we do it and print nothing,
> 
> 
> I thought the informational print would at least make the user aware
> of the fact that the separate filter request for BHRB went ignored.
> Can we add this some where in the documentation ?

So, what we decide here ? We will just ignore any separate BHRB
privilege state filter request without printing any informational
event or warning ?

Re: [PATCH 1/2] powerpc, perf: Ignore separate BHRB privilege state filter request

[Michael Neuling]

Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:

> On 05/22/2013 02:29 PM, Anshuman Khandual wrote:
> >>
> >> Your description from patch 0 should be here.
> > Does it sound better ?
> > 
> >>
> >>> -	if ((br_privilege != 7) && (br_privilege != 0))
> >>> -		return -1;
> >>> +
> >>> +	if (br_privilege)
> >>> +		pr_info("BHRB privilege state filter request %llx ignored\n",
> >>> +								br_privilege);
> >>
> >> Don't do that. Ignoring the br_privilege is either the right thing to do
> >> in which case we do it and print nothing,
> > 
> > 
> > I thought the informational print would at least make the user aware
> > of the fact that the separate filter request for BHRB went ignored.
> > Can we add this some where in the documentation ?
> 
> So, what we decide here ? We will just ignore any separate BHRB
> privilege state filter request without printing any informational
> event or warning ?

Printing it on the console is pointless.  No one will read it.  

If it's doing the filtering, then just drop the printk.  

	/* BHRB and regular PMU events share the same privilege state
	 * filter configuration. BHRB is always recorded along with a
	 * regular PMU event. As the privilege state filter is handled
	 * in the basic PMC configuration of the accompanying regular
	 * PMU event, we ignore any separate BHRB specific request.
	 */

That updated comment I think make it clear.  

So drop the printk and add the comment, and it's OK with me.

Mikey

[PATCH 2/2] powerpc, perf: BHRB filter configuration should follow the task

[Anshuman Khandual]

When the task moves around the system, the corresponding cpuhw
per cpu strcuture should be popullated with the BHRB filter
request value so that PMU could be configured appropriately with
that during the next call into power_pmu_enable().

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/perf/core-book3s.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 426180b..48c68a8 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -1122,8 +1122,11 @@ nocheck:
 
 	ret = 0;
  out:
-	if (has_branch_stack(event))
+	if (has_branch_stack(event)) {
 		power_pmu_bhrb_enable(event);
+		cpuhw->bhrb_filter = ppmu->bhrb_filter_map(
+					event->attr.branch_sample_type);
+	}
 
 	perf_pmu_enable(event->pmu);
 	local_irq_restore(flags);
-- 
1.7.11.7