Re: [PATCH V8 02/10] powerpc, perf: Restore privillege level filter support for BHRB

2015-06-12 Thread Anshuman Khandual 
On 06/11/2015 08:58 AM, Daniel Axtens wrote:
 
  
 -  if (!(ppmu-flags  PPMU_ARCH_207S)) {
 +  if (!(ppmu-flags  PPMU_ARCH_207S) || cpuhw-bhrb_users)

 You're using cpuhw-bhrb_users as a bool here, where it's an int. Could
 you make the test more specific so that it's clear exactly what you're
 expecting bhrb_users to contain?

 Using cpuhw-bhrb_users as a bool just verifies whether it contains
 zero or non-zero value in it. The test seems to be doing that as
 expected. But yes, we can move it as a nested conditional block as
 well if that is better.

 
 What I meant was, should this read (cpuhw-bhrb_users != 0)? Because
 bhrb_users in check_excludes() is a signed int, I also wanted to make
 sure it shouldn't be a test for bhrb_users  0 instead. (Also, if
 bhrb_users is always positive, should it be an unsigned int?)

Will replace both the conditional checks in comparison against 0.
Will change the data type of bhrb_users into unsigned int as well.

 
 I don't think a nested conditional would be better. 

Okay.

 
 
 
 -  if (check_excludes(ctrs, cflags, n, 1))
 +  cpuhw = get_cpu_var(cpu_hw_events);
 Should this be using a this_cpu_ptr rather than a get_cpu_var? (as with
 the power_pmu_commit_txn case?)
 +  if (check_excludes(ctrs, cflags, n, 1, cpuhw-bhrb_users)) {
 +  put_cpu_var(cpu_hw_events);
 Likewise with this?
return -EINVAL;
 +  }
  
 -  cpuhw = get_cpu_var(cpu_hw_events);

 This patch just moves the existing code couple of lines above without
 changing it in any manner.

 I see that, but I still think you should take this opportunity to
 improve it.

Will try to change it in a separate patch.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V8 02/10] powerpc, perf: Restore privillege level filter support for BHRB

2015-06-10 Thread Anshuman Khandual 
On 06/10/2015 09:13 AM, Daniel Axtens wrote:
 In the subject line, privilege should only have 1 l, and I think it
 should probably start with powerpc/perf: rather than powerpc, perf:.

Will fix the typo here. Have been using powerpc, perf: format for some
time now :) Seems to be more cleaner compared to powerpc/perf: format.
But again its subjective.

   On Mon, 2015-06-08 at 17:08 +0530, Anshuman Khandual wrote:
 From: khand...@linux.vnet.ibm.com khand...@linux.vnet.ibm.com

 'commit 9de5cb0f6df8 (powerpc/perf: Add per-event excludes on Power8)'
 Does this need a 'Fixes:' tag then?

Not really, it only fixes the BHRB privilege request cases not other
scenarios which are impacted by this previous commit.
 
 
 broke the PMU based BHRB privilege level filter. BHRB depends on the
 same MMCR0 bits for privilege level filter which was used to freeze all
 the PMCs as a group. Once we moved to individual event based privilege
 filters through MMCR2 register on POWER8, event associated privilege
 filters are no longer applicable to the BHRB captured branches.

 This patch solves the problem by restoring to the previous method of
 privilege level filters for the event in case BHRB based branch stack
 sampling is requested. This patch also changes 'check_excludes' for
 the same reason.

 Signed-off-by: Anshuman Khandual khand...@linux.vnet.ibm.com
 ---
  arch/powerpc/perf/core-book3s.c | 19 +++
  1 file changed, 11 insertions(+), 8 deletions(-)

 diff --git a/arch/powerpc/perf/core-book3s.c 
 b/arch/powerpc/perf/core-book3s.c
 index c246e65..ae61629 100644
 --- a/arch/powerpc/perf/core-book3s.c
 +++ b/arch/powerpc/perf/core-book3s.c
 @@ -930,7 +930,7 @@ static int power_check_constraints(struct cpu_hw_events 
 *cpuhw,
   * added events.
   */
 Does this comment need to be updated?

Not really. The previous commit did not update it, hence this patch would
skip it as well.

  static int check_excludes(struct perf_event **ctrs, unsigned int cflags[],
 -  int n_prev, int n_new)
 +  int n_prev, int n_new, int bhrb_users)
  {
  int eu = 0, ek = 0, eh = 0;
  int i, n, first;
 @@ -941,7 +941,7 @@ static int check_excludes(struct perf_event **ctrs, 
 unsigned int cflags[],
   * don't need to do any of this logic. NB. This assumes no PMU has both
   * per event exclude and limited PMCs.
   */
 Likewise, does this comment need to be updated?

Yeah, will update it.

 -if (ppmu-flags  PPMU_ARCH_207S)
 +if ((ppmu-flags  PPMU_ARCH_207S)  !bhrb_users)
  return 0;
  
  n = n_prev + n_new;
 @@ -1259,7 +1259,7 @@ static void power_pmu_enable(struct pmu *pmu)
  goto out;
  }
  
 -if (!(ppmu-flags  PPMU_ARCH_207S)) {
 +if (!(ppmu-flags  PPMU_ARCH_207S) || cpuhw-bhrb_users)

 You're using cpuhw-bhrb_users as a bool here, where it's an int. Could
 you make the test more specific so that it's clear exactly what you're
 expecting bhrb_users to contain?

Using cpuhw-bhrb_users as a bool just verifies whether it contains
zero or non-zero value in it. The test seems to be doing that as
expected. But yes, we can move it as a nested conditional block as
well if that is better.

  {
  /*
   * Add in MMCR0 freeze bits corresponding to the attr.exclude_*
   * bits for the first event. We have already checked that all
 @@ -1284,7 +1284,7 @@ static void power_pmu_enable(struct pmu *pmu)
  mtspr(SPRN_MMCR1, cpuhw-mmcr[1]);
  mtspr(SPRN_MMCR0, (cpuhw-mmcr[0]  ~(MMCR0_PMC1CE | MMCR0_PMCjCE))
  | MMCR0_FC);
 -if (ppmu-flags  PPMU_ARCH_207S)
 +if ((ppmu-flags  PPMU_ARCH_207S)  !cpuhw-bhrb_users)
  mtspr(SPRN_MMCR2, cpuhw-mmcr[3]);
  
  /*
 @@ -1436,7 +1436,8 @@ static int power_pmu_add(struct perf_event *event, int 
 ef_flags)
  if (cpuhw-group_flag  PERF_EVENT_TXN)
  goto nocheck;
  
 -if (check_excludes(cpuhw-event, cpuhw-flags, n0, 1))
 +if (check_excludes(cpuhw-event, cpuhw-flags,
 +n0, 1, cpuhw-bhrb_users))
  goto out;
  if (power_check_constraints(cpuhw, cpuhw-events, cpuhw-flags, n0 + 1))
  goto out;
 @@ -1615,7 +1616,7 @@ static int power_pmu_commit_txn(struct pmu *pmu)
  return -EAGAIN;
  cpuhw = this_cpu_ptr(cpu_hw_events);
  n = cpuhw-n_events;
 -if (check_excludes(cpuhw-event, cpuhw-flags, 0, n))
 +if (check_excludes(cpuhw-event, cpuhw-flags, 0, n, cpuhw-bhrb_users))
  return -EAGAIN;
  i = power_check_constraints(cpuhw, cpuhw-events, cpuhw-flags, n);
  if (i  0)
 @@ -1828,10 +1829,12 @@ static int power_pmu_event_init(struct perf_event 
 *event)
  events[n] = ev;
  ctrs[n] = event;
  cflags[n] = flags;
 -if (check_excludes(ctrs, cflags, n, 1))
 +cpuhw = get_cpu_var(cpu_hw_events);
 Should this be using a this_cpu_ptr rather than a get_cpu_var? (as with
 the power_pmu_commit_txn

Re: [PATCH V8 02/10] powerpc, perf: Restore privillege level filter support for BHRB

2015-06-10 Thread Daniel Axtens 

   
  -  if (!(ppmu-flags  PPMU_ARCH_207S)) {
  +  if (!(ppmu-flags  PPMU_ARCH_207S) || cpuhw-bhrb_users)
 
  You're using cpuhw-bhrb_users as a bool here, where it's an int. Could
  you make the test more specific so that it's clear exactly what you're
  expecting bhrb_users to contain?
 
 Using cpuhw-bhrb_users as a bool just verifies whether it contains
 zero or non-zero value in it. The test seems to be doing that as
 expected. But yes, we can move it as a nested conditional block as
 well if that is better.
 

What I meant was, should this read (cpuhw-bhrb_users != 0)? Because
bhrb_users in check_excludes() is a signed int, I also wanted to make
sure it shouldn't be a test for bhrb_users  0 instead. (Also, if
bhrb_users is always positive, should it be an unsigned int?)

I don't think a nested conditional would be better. 



  -  if (check_excludes(ctrs, cflags, n, 1))
  +  cpuhw = get_cpu_var(cpu_hw_events);
  Should this be using a this_cpu_ptr rather than a get_cpu_var? (as with
  the power_pmu_commit_txn case?)
  +  if (check_excludes(ctrs, cflags, n, 1, cpuhw-bhrb_users)) {
  +  put_cpu_var(cpu_hw_events);
  Likewise with this?
 return -EINVAL;
  +  }
   
  -  cpuhw = get_cpu_var(cpu_hw_events);
 
 This patch just moves the existing code couple of lines above without
 changing it in any manner.
 
I see that, but I still think you should take this opportunity to
improve it.

Regards,
Daniel


signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V8 02/10] powerpc, perf: Restore privillege level filter support for BHRB

2015-06-09 Thread Daniel Axtens 
In the subject line, privilege should only have 1 l, and I think it
should probably start with powerpc/perf: rather than powerpc, perf:.

On Mon, 2015-06-08 at 17:08 +0530, Anshuman Khandual wrote:
 From: khand...@linux.vnet.ibm.com khand...@linux.vnet.ibm.com
 
 'commit 9de5cb0f6df8 (powerpc/perf: Add per-event excludes on Power8)'
Does this need a 'Fixes:' tag then?

 broke the PMU based BHRB privilege level filter. BHRB depends on the
 same MMCR0 bits for privilege level filter which was used to freeze all
 the PMCs as a group. Once we moved to individual event based privilege
 filters through MMCR2 register on POWER8, event associated privilege
 filters are no longer applicable to the BHRB captured branches.
 
 This patch solves the problem by restoring to the previous method of
 privilege level filters for the event in case BHRB based branch stack
 sampling is requested. This patch also changes 'check_excludes' for
 the same reason.
 
 Signed-off-by: Anshuman Khandual khand...@linux.vnet.ibm.com
 ---
  arch/powerpc/perf/core-book3s.c | 19 +++
  1 file changed, 11 insertions(+), 8 deletions(-)
 
 diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
 index c246e65..ae61629 100644
 --- a/arch/powerpc/perf/core-book3s.c
 +++ b/arch/powerpc/perf/core-book3s.c
 @@ -930,7 +930,7 @@ static int power_check_constraints(struct cpu_hw_events 
 *cpuhw,
   * added events.
   */
Does this comment need to be updated?
  static int check_excludes(struct perf_event **ctrs, unsigned int cflags[],
 -   int n_prev, int n_new)
 +   int n_prev, int n_new, int bhrb_users)
  {
   int eu = 0, ek = 0, eh = 0;
   int i, n, first;
 @@ -941,7 +941,7 @@ static int check_excludes(struct perf_event **ctrs, 
 unsigned int cflags[],
* don't need to do any of this logic. NB. This assumes no PMU has both
* per event exclude and limited PMCs.
*/
Likewise, does this comment need to be updated?
 - if (ppmu-flags  PPMU_ARCH_207S)
 + if ((ppmu-flags  PPMU_ARCH_207S)  !bhrb_users)
   return 0;
  
   n = n_prev + n_new;
 @@ -1259,7 +1259,7 @@ static void power_pmu_enable(struct pmu *pmu)
   goto out;
   }
  
 - if (!(ppmu-flags  PPMU_ARCH_207S)) {
 + if (!(ppmu-flags  PPMU_ARCH_207S) || cpuhw-bhrb_users)
You're using cpuhw-bhrb_users as a bool here, where it's an int. Could
you make the test more specific so that it's clear exactly what you're
expecting bhrb_users to contain?
  {
   /*
* Add in MMCR0 freeze bits corresponding to the attr.exclude_*
* bits for the first event. We have already checked that all
 @@ -1284,7 +1284,7 @@ static void power_pmu_enable(struct pmu *pmu)
   mtspr(SPRN_MMCR1, cpuhw-mmcr[1]);
   mtspr(SPRN_MMCR0, (cpuhw-mmcr[0]  ~(MMCR0_PMC1CE | MMCR0_PMCjCE))
   | MMCR0_FC);
 - if (ppmu-flags  PPMU_ARCH_207S)
 + if ((ppmu-flags  PPMU_ARCH_207S)  !cpuhw-bhrb_users)
   mtspr(SPRN_MMCR2, cpuhw-mmcr[3]);
  
   /*
 @@ -1436,7 +1436,8 @@ static int power_pmu_add(struct perf_event *event, int 
 ef_flags)
   if (cpuhw-group_flag  PERF_EVENT_TXN)
   goto nocheck;
  
 - if (check_excludes(cpuhw-event, cpuhw-flags, n0, 1))
 + if (check_excludes(cpuhw-event, cpuhw-flags,
 + n0, 1, cpuhw-bhrb_users))
   goto out;
   if (power_check_constraints(cpuhw, cpuhw-events, cpuhw-flags, n0 + 1))
   goto out;
 @@ -1615,7 +1616,7 @@ static int power_pmu_commit_txn(struct pmu *pmu)
   return -EAGAIN;
   cpuhw = this_cpu_ptr(cpu_hw_events);
   n = cpuhw-n_events;
 - if (check_excludes(cpuhw-event, cpuhw-flags, 0, n))
 + if (check_excludes(cpuhw-event, cpuhw-flags, 0, n, cpuhw-bhrb_users))
   return -EAGAIN;
   i = power_check_constraints(cpuhw, cpuhw-events, cpuhw-flags, n);
   if (i  0)
 @@ -1828,10 +1829,12 @@ static int power_pmu_event_init(struct perf_event 
 *event)
   events[n] = ev;
   ctrs[n] = event;
   cflags[n] = flags;
 - if (check_excludes(ctrs, cflags, n, 1))
 + cpuhw = get_cpu_var(cpu_hw_events);
Should this be using a this_cpu_ptr rather than a get_cpu_var? (as with
the power_pmu_commit_txn case?)
 + if (check_excludes(ctrs, cflags, n, 1, cpuhw-bhrb_users)) {
 + put_cpu_var(cpu_hw_events);
Likewise with this?
   return -EINVAL;
 + }
  
 - cpuhw = get_cpu_var(cpu_hw_events);
   err = power_check_constraints(cpuhw, events, cflags, n + 1);
  
   if (has_branch_stack(event)) {



signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH V8 02/10] powerpc, perf: Restore privillege level filter support for BHRB

2015-06-08 Thread Anshuman Khandual 
From: khand...@linux.vnet.ibm.com khand...@linux.vnet.ibm.com

'commit 9de5cb0f6df8 (powerpc/perf: Add per-event excludes on Power8)'
broke the PMU based BHRB privilege level filter. BHRB depends on the
same MMCR0 bits for privilege level filter which was used to freeze all
the PMCs as a group. Once we moved to individual event based privilege
filters through MMCR2 register on POWER8, event associated privilege
filters are no longer applicable to the BHRB captured branches.

This patch solves the problem by restoring to the previous method of
privilege level filters for the event in case BHRB based branch stack
sampling is requested. This patch also changes 'check_excludes' for
the same reason.

Signed-off-by: Anshuman Khandual khand...@linux.vnet.ibm.com
---
 arch/powerpc/perf/core-book3s.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index c246e65..ae61629 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -930,7 +930,7 @@ static int power_check_constraints(struct cpu_hw_events 
*cpuhw,
  * added events.
  */
 static int check_excludes(struct perf_event **ctrs, unsigned int cflags[],
- int n_prev, int n_new)
+ int n_prev, int n_new, int bhrb_users)
 {
int eu = 0, ek = 0, eh = 0;
int i, n, first;
@@ -941,7 +941,7 @@ static int check_excludes(struct perf_event **ctrs, 
unsigned int cflags[],
 * don't need to do any of this logic. NB. This assumes no PMU has both
 * per event exclude and limited PMCs.
 */
-   if (ppmu-flags  PPMU_ARCH_207S)
+   if ((ppmu-flags  PPMU_ARCH_207S)  !bhrb_users)
return 0;
 
n = n_prev + n_new;
@@ -1259,7 +1259,7 @@ static void power_pmu_enable(struct pmu *pmu)
goto out;
}
 
-   if (!(ppmu-flags  PPMU_ARCH_207S)) {
+   if (!(ppmu-flags  PPMU_ARCH_207S) || cpuhw-bhrb_users) {
/*
 * Add in MMCR0 freeze bits corresponding to the attr.exclude_*
 * bits for the first event. We have already checked that all
@@ -1284,7 +1284,7 @@ static void power_pmu_enable(struct pmu *pmu)
mtspr(SPRN_MMCR1, cpuhw-mmcr[1]);
mtspr(SPRN_MMCR0, (cpuhw-mmcr[0]  ~(MMCR0_PMC1CE | MMCR0_PMCjCE))
| MMCR0_FC);
-   if (ppmu-flags  PPMU_ARCH_207S)
+   if ((ppmu-flags  PPMU_ARCH_207S)  !cpuhw-bhrb_users)
mtspr(SPRN_MMCR2, cpuhw-mmcr[3]);
 
/*
@@ -1436,7 +1436,8 @@ static int power_pmu_add(struct perf_event *event, int 
ef_flags)
if (cpuhw-group_flag  PERF_EVENT_TXN)
goto nocheck;
 
-   if (check_excludes(cpuhw-event, cpuhw-flags, n0, 1))
+   if (check_excludes(cpuhw-event, cpuhw-flags,
+   n0, 1, cpuhw-bhrb_users))
goto out;
if (power_check_constraints(cpuhw, cpuhw-events, cpuhw-flags, n0 + 1))
goto out;
@@ -1615,7 +1616,7 @@ static int power_pmu_commit_txn(struct pmu *pmu)
return -EAGAIN;
cpuhw = this_cpu_ptr(cpu_hw_events);
n = cpuhw-n_events;
-   if (check_excludes(cpuhw-event, cpuhw-flags, 0, n))
+   if (check_excludes(cpuhw-event, cpuhw-flags, 0, n, cpuhw-bhrb_users))
return -EAGAIN;
i = power_check_constraints(cpuhw, cpuhw-events, cpuhw-flags, n);
if (i  0)
@@ -1828,10 +1829,12 @@ static int power_pmu_event_init(struct perf_event 
*event)
events[n] = ev;
ctrs[n] = event;
cflags[n] = flags;
-   if (check_excludes(ctrs, cflags, n, 1))
+   cpuhw = get_cpu_var(cpu_hw_events);
+   if (check_excludes(ctrs, cflags, n, 1, cpuhw-bhrb_users)) {
+   put_cpu_var(cpu_hw_events);
return -EINVAL;
+   }
 
-   cpuhw = get_cpu_var(cpu_hw_events);
err = power_check_constraints(cpuhw, events, cflags, n + 1);
 
if (has_branch_stack(event)) {
-- 
2.1.0

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev