subject:"\[PATCH v3 4\/7\] MIPS\: perf\: Fix perf with MT counting other threads"

Re: [PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other threads

2018-05-17 Thread Matt Redfearn


Hi James,

On 16/05/18 18:59, James Hogan wrote:

On Fri, Apr 20, 2018 at 11:23:06AM +0100, Matt Redfearn wrote:

diff --git a/arch/mips/kernel/perf_event_mipsxx.c 
b/arch/mips/kernel/perf_event_mipsxx.c
index 7e2b7d38a774..fe50986e83c6 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -323,7 +323,11 @@ static int mipsxx_pmu_alloc_counter(struct cpu_hw_events 
*cpuc,
  
  static void mipsxx_pmu_enable_event(struct hw_perf_event *evt, int idx)

  {
+   struct perf_event *event = container_of(evt, struct perf_event, hw);
struct cpu_hw_events *cpuc = this_cpu_ptr(_hw_events);
+#ifdef CONFIG_MIPS_MT_SMP
+   unsigned int range = evt->event_base >> 24;
+#endif /* CONFIG_MIPS_MT_SMP */
  
  	WARN_ON(idx < 0 || idx >= mipspmu.num_counters);
  
@@ -331,11 +335,37 @@ static void mipsxx_pmu_enable_event(struct hw_perf_event *evt, int idx)

(evt->config_base & M_PERFCTL_CONFIG_MASK) |
/* Make sure interrupt enabled. */
MIPS_PERFCTRL_IE;
-   if (IS_ENABLED(CONFIG_CPU_BMIPS5000))
+
+#ifdef CONFIG_CPU_BMIPS5000
+   {
/* enable the counter for the calling thread */
cpuc->saved_ctrl[idx] |=
(1 << (12 + vpe_id())) | BRCM_PERFCTRL_TC;
+   }
+#else
+#ifdef CONFIG_MIPS_MT_SMP
+   if (range > V) {
+   /* The counter is processor wide. Set it up to count all TCs. */
+   pr_debug("Enabling perf counter for all TCs\n");
+   cpuc->saved_ctrl[idx] |= M_TC_EN_ALL;
+   } else
+#endif /* CONFIG_MIPS_MT_SMP */
+   {
+   unsigned int cpu, ctrl;
  
+		/*

+* Set up the counter for a particular CPU when event->cpu is
+* a valid CPU number. Otherwise set up the counter for the CPU
+* scheduling this thread.
+*/
+   cpu = (event->cpu >= 0) ? event->cpu : smp_processor_id();
+
+   ctrl = M_PERFCTL_VPEID(cpu_vpe_id(_data[cpu]));
+   ctrl |= M_TC_EN_VPE;
+   cpuc->saved_ctrl[idx] |= ctrl;
+   pr_debug("Enabling perf counter for CPU%d\n", cpu);
+   }
+#endif /* CONFIG_CPU_BMIPS5000 */


I'm not a huge fan of the ifdefery tbh, I don't think it makes it very
easy to read having a combination of ifs and #ifdefs. I reckon
IF_ENABLED would be better, perhaps with having the BMIPS5000 case
return to avoid too much nesting.


OK, I'll try and tidy it up.

Thanks,
Matt



Otherwise the patch looks okay to me.

Thanks
James

Re: [PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other threads

2018-05-17 Thread Matt Redfearn


Hi James,

On 16/05/18 18:59, James Hogan wrote:

On Fri, Apr 20, 2018 at 11:23:06AM +0100, Matt Redfearn wrote:

diff --git a/arch/mips/kernel/perf_event_mipsxx.c 
b/arch/mips/kernel/perf_event_mipsxx.c
index 7e2b7d38a774..fe50986e83c6 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -323,7 +323,11 @@ static int mipsxx_pmu_alloc_counter(struct cpu_hw_events 
*cpuc,
  
  static void mipsxx_pmu_enable_event(struct hw_perf_event *evt, int idx)

  {
+   struct perf_event *event = container_of(evt, struct perf_event, hw);
struct cpu_hw_events *cpuc = this_cpu_ptr(_hw_events);
+#ifdef CONFIG_MIPS_MT_SMP
+   unsigned int range = evt->event_base >> 24;
+#endif /* CONFIG_MIPS_MT_SMP */
  
  	WARN_ON(idx < 0 || idx >= mipspmu.num_counters);
  
@@ -331,11 +335,37 @@ static void mipsxx_pmu_enable_event(struct hw_perf_event *evt, int idx)

(evt->config_base & M_PERFCTL_CONFIG_MASK) |
/* Make sure interrupt enabled. */
MIPS_PERFCTRL_IE;
-   if (IS_ENABLED(CONFIG_CPU_BMIPS5000))
+
+#ifdef CONFIG_CPU_BMIPS5000
+   {
/* enable the counter for the calling thread */
cpuc->saved_ctrl[idx] |=
(1 << (12 + vpe_id())) | BRCM_PERFCTRL_TC;
+   }
+#else
+#ifdef CONFIG_MIPS_MT_SMP
+   if (range > V) {
+   /* The counter is processor wide. Set it up to count all TCs. */
+   pr_debug("Enabling perf counter for all TCs\n");
+   cpuc->saved_ctrl[idx] |= M_TC_EN_ALL;
+   } else
+#endif /* CONFIG_MIPS_MT_SMP */
+   {
+   unsigned int cpu, ctrl;
  
+		/*

+* Set up the counter for a particular CPU when event->cpu is
+* a valid CPU number. Otherwise set up the counter for the CPU
+* scheduling this thread.
+*/
+   cpu = (event->cpu >= 0) ? event->cpu : smp_processor_id();
+
+   ctrl = M_PERFCTL_VPEID(cpu_vpe_id(_data[cpu]));
+   ctrl |= M_TC_EN_VPE;
+   cpuc->saved_ctrl[idx] |= ctrl;
+   pr_debug("Enabling perf counter for CPU%d\n", cpu);
+   }
+#endif /* CONFIG_CPU_BMIPS5000 */


I'm not a huge fan of the ifdefery tbh, I don't think it makes it very
easy to read having a combination of ifs and #ifdefs. I reckon
IF_ENABLED would be better, perhaps with having the BMIPS5000 case
return to avoid too much nesting.


OK, I'll try and tidy it up.

Thanks,
Matt



Otherwise the patch looks okay to me.

Thanks
James

Re: [PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other threads

2018-05-16 Thread James Hogan

On Fri, Apr 20, 2018 at 11:23:06AM +0100, Matt Redfearn wrote:
> diff --git a/arch/mips/kernel/perf_event_mipsxx.c 
> b/arch/mips/kernel/perf_event_mipsxx.c
> index 7e2b7d38a774..fe50986e83c6 100644
> --- a/arch/mips/kernel/perf_event_mipsxx.c
> +++ b/arch/mips/kernel/perf_event_mipsxx.c
> @@ -323,7 +323,11 @@ static int mipsxx_pmu_alloc_counter(struct cpu_hw_events 
> *cpuc,
>  
>  static void mipsxx_pmu_enable_event(struct hw_perf_event *evt, int idx)
>  {
> + struct perf_event *event = container_of(evt, struct perf_event, hw);
>   struct cpu_hw_events *cpuc = this_cpu_ptr(_hw_events);
> +#ifdef CONFIG_MIPS_MT_SMP
> + unsigned int range = evt->event_base >> 24;
> +#endif /* CONFIG_MIPS_MT_SMP */
>  
>   WARN_ON(idx < 0 || idx >= mipspmu.num_counters);
>  
> @@ -331,11 +335,37 @@ static void mipsxx_pmu_enable_event(struct 
> hw_perf_event *evt, int idx)
>   (evt->config_base & M_PERFCTL_CONFIG_MASK) |
>   /* Make sure interrupt enabled. */
>   MIPS_PERFCTRL_IE;
> - if (IS_ENABLED(CONFIG_CPU_BMIPS5000))
> +
> +#ifdef CONFIG_CPU_BMIPS5000
> + {
>   /* enable the counter for the calling thread */
>   cpuc->saved_ctrl[idx] |=
>   (1 << (12 + vpe_id())) | BRCM_PERFCTRL_TC;
> + }
> +#else
> +#ifdef CONFIG_MIPS_MT_SMP
> + if (range > V) {
> + /* The counter is processor wide. Set it up to count all TCs. */
> + pr_debug("Enabling perf counter for all TCs\n");
> + cpuc->saved_ctrl[idx] |= M_TC_EN_ALL;
> + } else
> +#endif /* CONFIG_MIPS_MT_SMP */
> + {
> + unsigned int cpu, ctrl;
>  
> + /*
> +  * Set up the counter for a particular CPU when event->cpu is
> +  * a valid CPU number. Otherwise set up the counter for the CPU
> +  * scheduling this thread.
> +  */
> + cpu = (event->cpu >= 0) ? event->cpu : smp_processor_id();
> +
> + ctrl = M_PERFCTL_VPEID(cpu_vpe_id(_data[cpu]));
> + ctrl |= M_TC_EN_VPE;
> + cpuc->saved_ctrl[idx] |= ctrl;
> + pr_debug("Enabling perf counter for CPU%d\n", cpu);
> + }
> +#endif /* CONFIG_CPU_BMIPS5000 */

I'm not a huge fan of the ifdefery tbh, I don't think it makes it very
easy to read having a combination of ifs and #ifdefs. I reckon
IF_ENABLED would be better, perhaps with having the BMIPS5000 case
return to avoid too much nesting.

Otherwise the patch looks okay to me.

Thanks
James


signature.asc
Description: PGP signature

Re: [PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other threads

2018-05-16 Thread James Hogan

On Fri, Apr 20, 2018 at 11:23:06AM +0100, Matt Redfearn wrote:
> diff --git a/arch/mips/kernel/perf_event_mipsxx.c 
> b/arch/mips/kernel/perf_event_mipsxx.c
> index 7e2b7d38a774..fe50986e83c6 100644
> --- a/arch/mips/kernel/perf_event_mipsxx.c
> +++ b/arch/mips/kernel/perf_event_mipsxx.c
> @@ -323,7 +323,11 @@ static int mipsxx_pmu_alloc_counter(struct cpu_hw_events 
> *cpuc,
>  
>  static void mipsxx_pmu_enable_event(struct hw_perf_event *evt, int idx)
>  {
> + struct perf_event *event = container_of(evt, struct perf_event, hw);
>   struct cpu_hw_events *cpuc = this_cpu_ptr(_hw_events);
> +#ifdef CONFIG_MIPS_MT_SMP
> + unsigned int range = evt->event_base >> 24;
> +#endif /* CONFIG_MIPS_MT_SMP */
>  
>   WARN_ON(idx < 0 || idx >= mipspmu.num_counters);
>  
> @@ -331,11 +335,37 @@ static void mipsxx_pmu_enable_event(struct 
> hw_perf_event *evt, int idx)
>   (evt->config_base & M_PERFCTL_CONFIG_MASK) |
>   /* Make sure interrupt enabled. */
>   MIPS_PERFCTRL_IE;
> - if (IS_ENABLED(CONFIG_CPU_BMIPS5000))
> +
> +#ifdef CONFIG_CPU_BMIPS5000
> + {
>   /* enable the counter for the calling thread */
>   cpuc->saved_ctrl[idx] |=
>   (1 << (12 + vpe_id())) | BRCM_PERFCTRL_TC;
> + }
> +#else
> +#ifdef CONFIG_MIPS_MT_SMP
> + if (range > V) {
> + /* The counter is processor wide. Set it up to count all TCs. */
> + pr_debug("Enabling perf counter for all TCs\n");
> + cpuc->saved_ctrl[idx] |= M_TC_EN_ALL;
> + } else
> +#endif /* CONFIG_MIPS_MT_SMP */
> + {
> + unsigned int cpu, ctrl;
>  
> + /*
> +  * Set up the counter for a particular CPU when event->cpu is
> +  * a valid CPU number. Otherwise set up the counter for the CPU
> +  * scheduling this thread.
> +  */
> + cpu = (event->cpu >= 0) ? event->cpu : smp_processor_id();
> +
> + ctrl = M_PERFCTL_VPEID(cpu_vpe_id(_data[cpu]));
> + ctrl |= M_TC_EN_VPE;
> + cpuc->saved_ctrl[idx] |= ctrl;
> + pr_debug("Enabling perf counter for CPU%d\n", cpu);
> + }
> +#endif /* CONFIG_CPU_BMIPS5000 */

I'm not a huge fan of the ifdefery tbh, I don't think it makes it very
easy to read having a combination of ifs and #ifdefs. I reckon
IF_ENABLED would be better, perhaps with having the BMIPS5000 case
return to avoid too much nesting.

Otherwise the patch looks okay to me.

Thanks
James


signature.asc
Description: PGP signature

[PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other threads

2018-04-20 Thread Matt Redfearn

When perf is used in non-system mode, i.e. without specifying CPUs to
count on, check_and_calc_range falls into the case when it sets
M_TC_EN_ALL in the counter config_base. This has the impact of always
counting for all of the threads in a core, even when the user has not
requested it. For example this can be seen with a test program which
executes 30002 instructions and 1 branches running on one VPE and a
busy load on the other VPE in the core. Without this commit, the
expected count is not returned:

taskset 4 dd if=/dev/zero of=/dev/null count=10 & taskset 8 perf
stat -e instructions:u,branches:u ./test_prog

 Performance counter stats for './test_prog':

103235  instructions:u
 17015  branches:u

In order to fix this, remove check_and_calc_range entirely and perform
all of the logic in mipsxx_pmu_enable_event. Since
mipsxx_pmu_enable_event now requires the range of the event, ensure that
it is set by mipspmu_perf_event_encode in the same circumstances as
before (i.e. #ifdef CONFIG_MIPS_MT_SMP && num_possible_cpus() > 1).

The logic of mipsxx_pmu_enable_event now becomes:
If the CPU is a BMIPS5000, then use the special vpe_id() implementation
to select which VPE to count.
If the counter has a range greater than a single VPE, i.e. it is a
core-wide counter, then ensure that the counter is set up to count
events from all TCs (though, since this is true by definition, is this
necessary? Just enabling a core-wide counter in the per-VPE case appears
experimentally to return the same counts. This is left in for now as the
logic was present before).
If the event is set up to count a particular CPU (i.e. system mode),
then the VPE ID of that CPU is used for the counter.
Otherwise, the event should be counted on the CPU scheduling this thread
(this was the critical bit missing from the previous implementation) so
the VPE ID of this CPU is used for the counter.

With this commit, the same test as before returns the counts expected:

taskset 4 dd if=/dev/zero of=/dev/null count=10 & taskset 8 perf
stat -e instructions:u,branches:u ./test_prog

 Performance counter stats for './test_prog':

 30002  instructions:u
 1  branches:u

Signed-off-by: Matt Redfearn 

---

Changes in v3: None
Changes in v2:
Fix mipsxx_pmu_enable_event for !#ifdef CONFIG_MIPS_MT_SMP

 arch/mips/kernel/perf_event_mipsxx.c | 78 ++--
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/arch/mips/kernel/perf_event_mipsxx.c 
b/arch/mips/kernel/perf_event_mipsxx.c
index 7e2b7d38a774..fe50986e83c6 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -323,7 +323,11 @@ static int mipsxx_pmu_alloc_counter(struct cpu_hw_events 
*cpuc,
 
 static void mipsxx_pmu_enable_event(struct hw_perf_event *evt, int idx)
 {
+   struct perf_event *event = container_of(evt, struct perf_event, hw);
struct cpu_hw_events *cpuc = this_cpu_ptr(_hw_events);
+#ifdef CONFIG_MIPS_MT_SMP
+   unsigned int range = evt->event_base >> 24;
+#endif /* CONFIG_MIPS_MT_SMP */
 
WARN_ON(idx < 0 || idx >= mipspmu.num_counters);
 
@@ -331,11 +335,37 @@ static void mipsxx_pmu_enable_event(struct hw_perf_event 
*evt, int idx)
(evt->config_base & M_PERFCTL_CONFIG_MASK) |
/* Make sure interrupt enabled. */
MIPS_PERFCTRL_IE;
-   if (IS_ENABLED(CONFIG_CPU_BMIPS5000))
+
+#ifdef CONFIG_CPU_BMIPS5000
+   {
/* enable the counter for the calling thread */
cpuc->saved_ctrl[idx] |=
(1 << (12 + vpe_id())) | BRCM_PERFCTRL_TC;
+   }
+#else
+#ifdef CONFIG_MIPS_MT_SMP
+   if (range > V) {
+   /* The counter is processor wide. Set it up to count all TCs. */
+   pr_debug("Enabling perf counter for all TCs\n");
+   cpuc->saved_ctrl[idx] |= M_TC_EN_ALL;
+   } else
+#endif /* CONFIG_MIPS_MT_SMP */
+   {
+   unsigned int cpu, ctrl;
 
+   /*
+* Set up the counter for a particular CPU when event->cpu is
+* a valid CPU number. Otherwise set up the counter for the CPU
+* scheduling this thread.
+*/
+   cpu = (event->cpu >= 0) ? event->cpu : smp_processor_id();
+
+   ctrl = M_PERFCTL_VPEID(cpu_vpe_id(_data[cpu]));
+   ctrl |= M_TC_EN_VPE;
+   cpuc->saved_ctrl[idx] |= ctrl;
+   pr_debug("Enabling perf counter for CPU%d\n", cpu);
+   }
+#endif /* CONFIG_CPU_BMIPS5000 */
/*
 * We do not actually let the counter run. Leave it until start().
 */
@@ -649,13 +679,14 @@ static unsigned int mipspmu_perf_event_encode(const 
struct mips_perf_event *pev)
  * event_id.
  */
 #ifdef CONFIG_MIPS_MT_SMP
-   return ((unsigned int)pev->range << 24) |
-   (pev->cntr_mask &

[PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other threads

2018-04-20 Thread Matt Redfearn

When perf is used in non-system mode, i.e. without specifying CPUs to
count on, check_and_calc_range falls into the case when it sets
M_TC_EN_ALL in the counter config_base. This has the impact of always
counting for all of the threads in a core, even when the user has not
requested it. For example this can be seen with a test program which
executes 30002 instructions and 1 branches running on one VPE and a
busy load on the other VPE in the core. Without this commit, the
expected count is not returned:

taskset 4 dd if=/dev/zero of=/dev/null count=10 & taskset 8 perf
stat -e instructions:u,branches:u ./test_prog

 Performance counter stats for './test_prog':

103235  instructions:u
 17015  branches:u

In order to fix this, remove check_and_calc_range entirely and perform
all of the logic in mipsxx_pmu_enable_event. Since
mipsxx_pmu_enable_event now requires the range of the event, ensure that
it is set by mipspmu_perf_event_encode in the same circumstances as
before (i.e. #ifdef CONFIG_MIPS_MT_SMP && num_possible_cpus() > 1).

The logic of mipsxx_pmu_enable_event now becomes:
If the CPU is a BMIPS5000, then use the special vpe_id() implementation
to select which VPE to count.
If the counter has a range greater than a single VPE, i.e. it is a
core-wide counter, then ensure that the counter is set up to count
events from all TCs (though, since this is true by definition, is this
necessary? Just enabling a core-wide counter in the per-VPE case appears
experimentally to return the same counts. This is left in for now as the
logic was present before).
If the event is set up to count a particular CPU (i.e. system mode),
then the VPE ID of that CPU is used for the counter.
Otherwise, the event should be counted on the CPU scheduling this thread
(this was the critical bit missing from the previous implementation) so
the VPE ID of this CPU is used for the counter.

With this commit, the same test as before returns the counts expected:

taskset 4 dd if=/dev/zero of=/dev/null count=10 & taskset 8 perf
stat -e instructions:u,branches:u ./test_prog

 Performance counter stats for './test_prog':

 30002  instructions:u
 1  branches:u

Signed-off-by: Matt Redfearn 

---

Changes in v3: None
Changes in v2:
Fix mipsxx_pmu_enable_event for !#ifdef CONFIG_MIPS_MT_SMP

 arch/mips/kernel/perf_event_mipsxx.c | 78 ++--
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/arch/mips/kernel/perf_event_mipsxx.c 
b/arch/mips/kernel/perf_event_mipsxx.c
index 7e2b7d38a774..fe50986e83c6 100644
--- a/arch/mips/kernel/perf_event_mipsxx.c
+++ b/arch/mips/kernel/perf_event_mipsxx.c
@@ -323,7 +323,11 @@ static int mipsxx_pmu_alloc_counter(struct cpu_hw_events 
*cpuc,
 
 static void mipsxx_pmu_enable_event(struct hw_perf_event *evt, int idx)
 {
+   struct perf_event *event = container_of(evt, struct perf_event, hw);
struct cpu_hw_events *cpuc = this_cpu_ptr(_hw_events);
+#ifdef CONFIG_MIPS_MT_SMP
+   unsigned int range = evt->event_base >> 24;
+#endif /* CONFIG_MIPS_MT_SMP */
 
WARN_ON(idx < 0 || idx >= mipspmu.num_counters);
 
@@ -331,11 +335,37 @@ static void mipsxx_pmu_enable_event(struct hw_perf_event 
*evt, int idx)
(evt->config_base & M_PERFCTL_CONFIG_MASK) |
/* Make sure interrupt enabled. */
MIPS_PERFCTRL_IE;
-   if (IS_ENABLED(CONFIG_CPU_BMIPS5000))
+
+#ifdef CONFIG_CPU_BMIPS5000
+   {
/* enable the counter for the calling thread */
cpuc->saved_ctrl[idx] |=
(1 << (12 + vpe_id())) | BRCM_PERFCTRL_TC;
+   }
+#else
+#ifdef CONFIG_MIPS_MT_SMP
+   if (range > V) {
+   /* The counter is processor wide. Set it up to count all TCs. */
+   pr_debug("Enabling perf counter for all TCs\n");
+   cpuc->saved_ctrl[idx] |= M_TC_EN_ALL;
+   } else
+#endif /* CONFIG_MIPS_MT_SMP */
+   {
+   unsigned int cpu, ctrl;
 
+   /*
+* Set up the counter for a particular CPU when event->cpu is
+* a valid CPU number. Otherwise set up the counter for the CPU
+* scheduling this thread.
+*/
+   cpu = (event->cpu >= 0) ? event->cpu : smp_processor_id();
+
+   ctrl = M_PERFCTL_VPEID(cpu_vpe_id(_data[cpu]));
+   ctrl |= M_TC_EN_VPE;
+   cpuc->saved_ctrl[idx] |= ctrl;
+   pr_debug("Enabling perf counter for CPU%d\n", cpu);
+   }
+#endif /* CONFIG_CPU_BMIPS5000 */
/*
 * We do not actually let the counter run. Leave it until start().
 */
@@ -649,13 +679,14 @@ static unsigned int mipspmu_perf_event_encode(const 
struct mips_perf_event *pev)
  * event_id.
  */
 #ifdef CONFIG_MIPS_MT_SMP
-   return ((unsigned int)pev->range << 24) |
-   (pev->cntr_mask & 0x00) |
-

Re: [PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other threads

Re: [PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other threads

Re: [PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other threads

Re: [PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other threads

[PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other threads

[PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other threads

6 matches

Site Navigation

Mail list logo

Footer information