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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 May 2018 18:59:18 +0100
From:   James Hogan <jhogan@...nel.org>
To:     Matt Redfearn <matt.redfearn@...s.com>
Cc:     Ralf Baechle <ralf@...ux-mips.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        linux-mips@...ux-mips.org, Namhyung Kim <namhyung@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>
Subject: Re: [PATCH v3 4/7] MIPS: perf: Fix perf with MT counting other
 threads

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(&cpu_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(&cpu_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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)