Re: [PATCH v3 0/7] MIPS: perf: MT fixes and improvements

On 20/04/18 23:51, Florian Fainelli wrote:
> On 04/20/2018 03:23 AM, Matt Redfearn wrote:
>> This series addresses a few issues with how the MIPS performance
>> counters code supports the hardware multithreading MT ASE.
>>
>> Firstly, implementations of the MT ASE may implement performance
>> counters
>> per core or per thread(TC). MIPS Techologies implementations signal this
>> via a bit in the implmentation specific CONFIG7 register. Since this
>> register is implementation specific, checking it should be guarded by a
>> PRID check. This also replaces a bit defined by a magic number.
>>
>> Secondly, the code currently uses vpe_id(), defined as
>> smp_processor_id(), divided by 2, to share core performance counters
>> between VPEs. This relies on a couple of assumptions of the hardware
>> implementation to function correctly (always 2 VPEs per core, and the
>> hardware reading only the least significant bit).
>>
>> Finally, the method of sharing core performance counters between VPEs is
>> suboptimal since it does not allow one process running on a VPE to use
>> all of the performance counters available to it, because the kernel will
>> reserve half of the coutners for the other VPE even if it may never use
>> them. This reservation at counter probe is replaced with an allocation
>> on use strategy.
>>
>> Tested on a MIPS Creator CI40 (2C2T MIPS InterAptiv with per-TC
>> counters, though for the purposes of testing the per-TC availability was
>> hardcoded to allow testing both paths).
>>
>> Series applies to v4.16
> 
> Sorry it took so long to get that tested.

Hi Florian,

Thanks for testing!

> 
> Sounds like you need to build test this on a BMIPS5000 configuration
> (bmips_stb_defconfig should provide that):
> 
> In file included from ./arch/mips/include/asm/mach-generic/spaces.h:15:0,
>                   from ./arch/mips/include/asm/mach-bmips/spaces.h:16,
>                   from ./arch/mips/include/asm/addrspace.h:13,
>                   from ./arch/mips/include/asm/barrier.h:11,
>                   from ./include/linux/compiler.h:245,
>                   from ./include/linux/kernel.h:10,
>                   from ./include/linux/cpumask.h:10,
>                   from arch/mips/kernel/perf_event_mipsxx.c:18:
> arch/mips/kernel/perf_event_mipsxx.c: In function 'mipsxx_pmu_enable_event':
> ./arch/mips/include/asm/mipsregs.h:738:52: error: suggest parentheses
> around '+' in operand of '&' [-Werror=parentheses]
>   #define BRCM_PERFCTRL_VPEID(v) (_ULCAST_(1) << (12 + v))
> 
> arch/mips/kernel/perf_event_mipsxx.c:385:10: note: in expansion of macro
> 'BRCM_PERFCTRL_VPEID'
>     ctrl = BRCM_PERFCTRL_VPEID(cpu & MIPS_CPUID_TO_COUNTER_MASK);
>            ^~~~~~~~~~~~~~~~~~~
>    CC      drivers/of/fdt_addres

Good spot - I've updated the patch to

+#define BRCM_PERFCTRL_VPEID(v) (_ULCAST_(1) << (12 + (v)))

to fix that.

> 
> after fixing that, I tried the following to see whether this would be a
> good test case to exercise against:
> 
> perf record -a -C 0 taskset -c 1 /bin/true
> perf record -a -C 1 taskset -c 0 /bin/true
> 
> and would not see anything related to /bin/true running in either case,
> which seems like it does the right thing?

I've generally been testing using this code:

perf_test.S:

#include <asm/unistd.h>

#define ITERATIONS 10000


.text
.global __start
.type	__start, @function;
__start:
	.set	noreorder
	li      $2, ITERATIONS

1:
	addiu   $2,$2,-1
	bnez    $2, 1b
	 nop

	li	$2, __NR_exit
	syscall


Makefile:
$(CC) $(ISA_FLAG) $(ABI_FLAG) $(ENDIAN_FLAG) -static -nostartfiles -O2 
-o perf_test perf_test.S

Then running perf which should report the right counts:

taskset 1 perf stat -e instructions:u,branches:u ./perf_test
  Performance counter stats for './perf_test':

              30002      instructions:u
              10000      branches:u

        0.005179467 seconds time elapsed


System mode should also work:

# perf stat -e instructions:u,branches:u -a -C 2
^C

  Performance counter stats for 'system wide':

               1416      instructions:u 

                198      branches:u 


        4.454874812 seconds time elapsed

> 
> Tested-by: Florian Fainelli <[email protected]>

Thanks!

> 
> BTW, for some reason not specifying -a -C <cpu> does lead to lockups,
> consistently and for pretty much any perf command, this could be BMIPS
> specific, did not get a chance to cross test on a different machine.

Interesting.... FWIW I don't get lockups on ci40 (MIPS InterAptiv). Is 
this a regression with this series applied or an existing problem?

Thanks,
Matt

> 
>>
>>
>> Changes in v3:
>> New patch to detect feature presence in cpu-probe.c
>> Use flag in cpu_data set by cpu_probe.c to indicate feature presence.
>> - rebase on new feature detection
>>
>> Changes in v2:
>> Fix mipsxx_pmu_enable_event for !#ifdef CONFIG_MIPS_MT_SMP
>> - Fix !#ifdef CONFIG_MIPS_PERF_SHARED_TC_COUNTERS build
>> - re-use cpuc variable in mipsxx_pmu_alloc_counter,
>>    mipsxx_pmu_free_counter rather than having sibling_ version.
>> Since BMIPS5000 does not implement per TC counters, we can remove the
>> check on cpu_has_mipsmt_pertccounters.
>> New patch to fix BMIPS5000 system mode perf.
>>
>> Matt Redfearn (7):
>>    MIPS: Probe for MIPS MT perf counters per TC
>>    MIPS: perf: More robustly probe for the presence of per-tc counters
>>    MIPS: perf: Use correct VPE ID when setting up VPE tracing
>>    MIPS: perf: Fix perf with MT counting other threads
>>    MIPS: perf: Allocate per-core counters on demand
>>    MIPS: perf: Fold vpe_id() macro into it's one last usage
>>    MIPS: perf: Fix BMIPS5000 system mode counting
>>
>>   arch/mips/include/asm/cpu-features.h |   7 ++
>>   arch/mips/include/asm/cpu.h          |   2 +
>>   arch/mips/include/asm/mipsregs.h     |   6 +
>>   arch/mips/kernel/cpu-probe.c         |  12 ++
>>   arch/mips/kernel/perf_event_mipsxx.c | 232 +++++++++++++++++++----------------
>>   arch/mips/oprofile/op_model_mipsxx.c |   2 -
>>   6 files changed, 150 insertions(+), 111 deletions(-)
>>
> 
>