[PATCH V3 0/2] Improvement and fixes for BHRB

[PATCH V3 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

Changes in V2
-------------
(1) Updated the comment section of the code
(2) Removed the informational print
(3) Updated the commit section of the first patch

Changes in V3
-------------
(1) Fixed the compilation warning problem after removing the unused variable

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  | 13 ++++---------
 2 files changed, 8 insertions(+), 10 deletions(-)

-- 
1.7.11.7

[PATCH V3 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 that with privilege state filtering attribute
for the accompanying PMU event. This would help achieve cleaner
user space interaction for BHRB.

This 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)]

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

diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
index f7d1c4f..371c6e7 100644
--- a/arch/powerpc/perf/power8-pmu.c
+++ b/arch/powerpc/perf/power8-pmu.c
@@ -523,18 +523,13 @@ static int power8_generic_events[] = {
 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 privilege 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. 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.
 	 */
-	if ((br_privilege != 7) && (br_privilege != 0))
-		return -1;
 
 	/* No branch filter requested */
 	if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY)
-- 
1.7.11.7

[PATCH V3 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

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

[Michael Neuling]

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

> Completely ignore BHRB privilege state filter request as we are
> already configuring that with privilege state filtering attribute
> for the accompanying PMU event. This would help achieve cleaner
> user space interaction for BHRB.
> 
> This 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)]
> 
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>

Acked-by: Michael Neuling <mikey@neuling.org>


> ---
>  arch/powerpc/perf/power8-pmu.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
> index f7d1c4f..371c6e7 100644
> --- a/arch/powerpc/perf/power8-pmu.c
> +++ b/arch/powerpc/perf/power8-pmu.c
> @@ -523,18 +523,13 @@ static int power8_generic_events[] = {
>  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 privilege 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. 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.
>  	 */
> -	if ((br_privilege != 7) && (br_privilege != 0))
> -		return -1;
>  
>  	/* No branch filter requested */
>  	if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY)
> -- 
> 1.7.11.7
> 

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

[Michael Neuling]

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

> 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>

benh you might want to fix the spelling mistakes above 

  strcuture -> structure
  popullated -> populated

Otherwise:

Acked-by: Michael Neuling <mikey@neuling.org>


> ---
>  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
> 

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

[Anshuman Khandual]

On 06/24/2013 12:50 PM, Michael Neuling wrote:
> Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote:
> 
>> 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>
> 
> benh you might want to fix the spelling mistakes above 
> 
>   strcuture -> structure
>   popullated -> populated
> 
> Otherwise:
> 
> Acked-by: Michael Neuling <mikey@neuling.org>

Hey Ben,

These patches have not been applied yet. Would like me to correct
these spellings myself and send out again. Please do let me know. Thanks !

Regards
Anshuman

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

[Benjamin Herrenschmidt]

On Mon, 2013-07-22 at 12:09 +0530, Anshuman Khandual wrote:
> These patches have not been applied yet. Would like me to correct
> these spellings myself and send out again. Please do let me know.
> Thanks !

No, it's ok, I'll add them to my batch.

Usually I assume that perf related stuff goes through acme's perf tree
so I might have overlooked them because of that.

Michael, you're ok with these ?

Cheers,
Ben

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

[Michael Ellerman]

On Mon, Jul 22, 2013 at 04:48:16PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2013-07-22 at 12:09 +0530, Anshuman Khandual wrote:
> > These patches have not been applied yet. Would like me to correct
> > these spellings myself and send out again. Please do let me know.
> > Thanks !
> 
> No, it's ok, I'll add them to my batch.
> 
> Usually I assume that perf related stuff goes through acme's perf tree
> so I might have overlooked them because of that.
> 
> Michael, you're ok with these ?

Which Michael? :)

I haven't had time to really get my head around the BHRB code, so I
can't really say.

cheers

[PATCH V3 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