Re: [PATCH 3/5] MIPS: perf: Fix perf with MT counting other threads
Hi Matt, Thank you for the patch! Yet something to improve: [auto build test ERROR on tip/perf/core] [also build test ERROR on v4.16 next-20180403] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Matt-Redfearn/MIPS-perf-MT-fixes-and-improvements/20180404-011026 config: mips-gpr_defconfig (attached as .config) compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=mips All errors (new ones prefixed by >>): arch/mips/kernel/perf_event_mipsxx.c: In function 'mipsxx_pmu_enable_event': >> arch/mips/kernel/perf_event_mipsxx.c:343:22: error: expected expression >> before ')' token } else if (range > V) { ^ arch/mips/kernel/perf_event_mipsxx.c: In function 'mipspmu_perf_event_encode': >> arch/mips/kernel/perf_event_mipsxx.c:675:28: error: 'const struct >> mips_perf_event' has no member named 'range' return ((unsigned int)pev->range << 24) | ^~ vim +343 arch/mips/kernel/perf_event_mipsxx.c 325 326 static void mipsxx_pmu_enable_event(struct hw_perf_event *evt, int idx) 327 { 328 struct perf_event *event = container_of(evt, struct perf_event, hw); 329 struct cpu_hw_events *cpuc = this_cpu_ptr(_hw_events); 330 unsigned int range = evt->event_base >> 24; 331 332 WARN_ON(idx < 0 || idx >= mipspmu.num_counters); 333 334 cpuc->saved_ctrl[idx] = M_PERFCTL_EVENT(evt->event_base & 0xff) | 335 (evt->config_base & M_PERFCTL_CONFIG_MASK) | 336 /* Make sure interrupt enabled. */ 337 MIPS_PERFCTRL_IE; 338 339 if (IS_ENABLED(CONFIG_CPU_BMIPS5000)) { 340 /* enable the counter for the calling thread */ 341 cpuc->saved_ctrl[idx] |= 342 (1 << (12 + vpe_id())) | BRCM_PERFCTRL_TC; > 343 } else if (range > V) { 344 /* The counter is processor wide. Set it up to count all TCs. */ 345 pr_debug("Enabling perf counter for all TCs\n"); 346 cpuc->saved_ctrl[idx] |= M_TC_EN_ALL; 347 } else { 348 unsigned int cpu, ctrl; 349 350 /* 351 * Set up the counter for a particular CPU when event->cpu is 352 * a valid CPU number. Otherwise set up the counter for the CPU 353 * scheduling this thread. 354 */ 355 cpu = (event->cpu >= 0) ? event->cpu : smp_processor_id(); 356 357 ctrl = M_PERFCTL_VPEID(cpu_vpe_id(_data[cpu])); 358 ctrl |= M_TC_EN_VPE; 359 cpuc->saved_ctrl[idx] |= ctrl; 360 pr_debug("Enabling perf counter for CPU%d\n", cpu); 361 } 362 /* 363 * We do not actually let the counter run. Leave it until start(). 364 */ 365 } 366 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH 3/5] MIPS: perf: Fix perf with MT counting other threads
Hi Matt, Thank you for the patch! Yet something to improve: [auto build test ERROR on tip/perf/core] [also build test ERROR on v4.16 next-20180403] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Matt-Redfearn/MIPS-perf-MT-fixes-and-improvements/20180404-011026 config: mips-gpr_defconfig (attached as .config) compiler: mipsel-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=mips All errors (new ones prefixed by >>): arch/mips/kernel/perf_event_mipsxx.c: In function 'mipsxx_pmu_enable_event': >> arch/mips/kernel/perf_event_mipsxx.c:343:22: error: expected expression >> before ')' token } else if (range > V) { ^ arch/mips/kernel/perf_event_mipsxx.c: In function 'mipspmu_perf_event_encode': >> arch/mips/kernel/perf_event_mipsxx.c:675:28: error: 'const struct >> mips_perf_event' has no member named 'range' return ((unsigned int)pev->range << 24) | ^~ vim +343 arch/mips/kernel/perf_event_mipsxx.c 325 326 static void mipsxx_pmu_enable_event(struct hw_perf_event *evt, int idx) 327 { 328 struct perf_event *event = container_of(evt, struct perf_event, hw); 329 struct cpu_hw_events *cpuc = this_cpu_ptr(_hw_events); 330 unsigned int range = evt->event_base >> 24; 331 332 WARN_ON(idx < 0 || idx >= mipspmu.num_counters); 333 334 cpuc->saved_ctrl[idx] = M_PERFCTL_EVENT(evt->event_base & 0xff) | 335 (evt->config_base & M_PERFCTL_CONFIG_MASK) | 336 /* Make sure interrupt enabled. */ 337 MIPS_PERFCTRL_IE; 338 339 if (IS_ENABLED(CONFIG_CPU_BMIPS5000)) { 340 /* enable the counter for the calling thread */ 341 cpuc->saved_ctrl[idx] |= 342 (1 << (12 + vpe_id())) | BRCM_PERFCTRL_TC; > 343 } else if (range > V) { 344 /* The counter is processor wide. Set it up to count all TCs. */ 345 pr_debug("Enabling perf counter for all TCs\n"); 346 cpuc->saved_ctrl[idx] |= M_TC_EN_ALL; 347 } else { 348 unsigned int cpu, ctrl; 349 350 /* 351 * Set up the counter for a particular CPU when event->cpu is 352 * a valid CPU number. Otherwise set up the counter for the CPU 353 * scheduling this thread. 354 */ 355 cpu = (event->cpu >= 0) ? event->cpu : smp_processor_id(); 356 357 ctrl = M_PERFCTL_VPEID(cpu_vpe_id(_data[cpu])); 358 ctrl |= M_TC_EN_VPE; 359 cpuc->saved_ctrl[idx] |= ctrl; 360 pr_debug("Enabling perf counter for CPU%d\n", cpu); 361 } 362 /* 363 * We do not actually let the counter run. Leave it until start(). 364 */ 365 } 366 --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
[PATCH 3/5] MIPS: perf: Fix perf with MT counting other threads
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. IS_ENABLED(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--- arch/mips/kernel/perf_event_mipsxx.c | 69 +++- 1 file changed, 29 insertions(+), 40 deletions(-) diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c index bb8c6783e9a5..782a1b6c6352 100644 --- a/arch/mips/kernel/perf_event_mipsxx.c +++ b/arch/mips/kernel/perf_event_mipsxx.c @@ -325,7 +325,9 @@ 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); + unsigned int range = evt->event_base >> 24; WARN_ON(idx < 0 || idx >= mipspmu.num_counters); @@ -333,11 +335,30 @@ 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)) + + if (IS_ENABLED(CONFIG_CPU_BMIPS5000)) { /* enable the counter for the calling thread */ cpuc->saved_ctrl[idx] |= (1 << (12 + vpe_id())) | BRCM_PERFCTRL_TC; + } else 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 { + 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); + } /* * We do not actually let the counter run. Leave it until start(). */ @@ -650,14 +671,13 @@ static unsigned int mipspmu_perf_event_encode(const struct mips_perf_event *pev) * Top 8 bits for range, next 16 bits for cntr_mask, lowest 8 bits for * event_id. */ -#ifdef CONFIG_MIPS_MT_SMP - return ((unsigned int)pev->range << 24) | - (pev->cntr_mask & 0x00) | - (pev->event_id & 0xff); -#else - return (pev->cntr_mask & 0x00) | - (pev->event_id & 0xff); -#endif + if
[PATCH 3/5] MIPS: perf: Fix perf with MT counting other threads
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. IS_ENABLED(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 --- arch/mips/kernel/perf_event_mipsxx.c | 69 +++- 1 file changed, 29 insertions(+), 40 deletions(-) diff --git a/arch/mips/kernel/perf_event_mipsxx.c b/arch/mips/kernel/perf_event_mipsxx.c index bb8c6783e9a5..782a1b6c6352 100644 --- a/arch/mips/kernel/perf_event_mipsxx.c +++ b/arch/mips/kernel/perf_event_mipsxx.c @@ -325,7 +325,9 @@ 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); + unsigned int range = evt->event_base >> 24; WARN_ON(idx < 0 || idx >= mipspmu.num_counters); @@ -333,11 +335,30 @@ 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)) + + if (IS_ENABLED(CONFIG_CPU_BMIPS5000)) { /* enable the counter for the calling thread */ cpuc->saved_ctrl[idx] |= (1 << (12 + vpe_id())) | BRCM_PERFCTRL_TC; + } else 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 { + 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); + } /* * We do not actually let the counter run. Leave it until start(). */ @@ -650,14 +671,13 @@ static unsigned int mipspmu_perf_event_encode(const struct mips_perf_event *pev) * Top 8 bits for range, next 16 bits for cntr_mask, lowest 8 bits for * event_id. */ -#ifdef CONFIG_MIPS_MT_SMP - return ((unsigned int)pev->range << 24) | - (pev->cntr_mask & 0x00) | - (pev->event_id & 0xff); -#else - return (pev->cntr_mask & 0x00) | - (pev->event_id & 0xff); -#endif + if (IS_ENABLED(CONFIG_MIPS_MT_SMP) &&