Re: [PATCH 2/3] x86/topology: Fix AMD core count

[Borislav Petkov]

On Sun, Mar 20, 2016 at 01:46:29PM +0100, Peter Zijlstra wrote:
> On Sun, Mar 20, 2016 at 01:32:25PM +0100, Peter Zijlstra wrote:
> > Yes, but IIRC the F15h driver doesn't use the NB constraints, as they
> > moved all the NB events to their own set of MSRs, which has
> > events/amd/uncore.c.
> > 
> > So all the NB cruft in the core pmu is only relevant to F10h.
> > 
> > So F15h also calling and allocating NB cruft in the core PMU driver is
> > entirely pointless.
> 
> Something like so.

Perhaps. Some minor notes below.

First a question about the big picture: why is amd/core.c even
dealing with NB counters? Especially if NB has its own initcall
amd_uncore_init()?

IOW, what I'm trying to say is, can we untangle uncore.c from core.c or
is there some dependency in the modelling I'm not aware of... which is
very likely, btw.

> ---
>  arch/x86/events/amd/core.c   | 21 ++++++++++++++++++---
>  arch/x86/events/perf_event.h |  5 +++++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index 049ada8d4e9c..62026a79a930 100644
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
> @@ -369,7 +369,7 @@ static int amd_pmu_cpu_prepare(int cpu)
>  
>  	WARN_ON_ONCE(cpuc->amd_nb);
>  
> -	if (boot_cpu_data.x86_max_cores < 2)
> +	if (!x86_pmu.amd_nb)

Yeah, except there's cpuc->amd_nb and now pmu->amd_nb. Can we
differentiate a bit more with the naming?

>  		return NOTIFY_OK;
>  
>  	cpuc->amd_nb = amd_alloc_nb(cpu);
> @@ -388,7 +388,7 @@ static void amd_pmu_cpu_starting(int cpu)
>  
>  	cpuc->perf_ctr_virt_mask = AMD64_EVENTSEL_HOSTONLY;
>  
> -	if (boot_cpu_data.x86_max_cores < 2)
> +	if (!x86_pmu.amd_nb)
>  		return;
>  
>  	nb_id = amd_get_nb_id(cpu);
> @@ -414,7 +414,7 @@ static void amd_pmu_cpu_dead(int cpu)
>  {
>  	struct cpu_hw_events *cpuhw;
>  
> -	if (boot_cpu_data.x86_max_cores < 2)
> +	if (!x86_pmu.amd_nb)
>  		return;
>  
>  	cpuhw = &per_cpu(cpu_hw_events, cpu);
> @@ -648,6 +648,8 @@ static __initconst const struct x86_pmu amd_pmu = {
>  	.cpu_prepare		= amd_pmu_cpu_prepare,
>  	.cpu_starting		= amd_pmu_cpu_starting,
>  	.cpu_dead		= amd_pmu_cpu_dead,
> +
> +	.amd_nb			= 1;

s/;/,/

>  };
>  
>  static int __init amd_core_pmu_init(void)
> @@ -674,6 +676,11 @@ static int __init amd_core_pmu_init(void)
>  	x86_pmu.eventsel	= MSR_F15H_PERF_CTL;
>  	x86_pmu.perfctr		= MSR_F15H_PERF_CTR;
>  	x86_pmu.num_counters	= AMD64_NUM_COUNTERS_CORE;
> +	/*
> +	 * AMD Core perfctr has separate MSRs for the NB events, see
> +	 * the amd/uncore.c driver.
> +	 */
> +	x86_pmu.amd_nb		= 0;
>  
>  	pr_cont("core perfctr, ");
>  	return 0;
> @@ -693,6 +700,14 @@ __init int amd_pmu_init(void)
>  	if (ret)
>  		return ret;
>  
> +	if (num_possible_cpus() == 1) {
> +		/*
> +		 * No point in allocating data structures to serialize
> +		 * against other CPUs, when there is only the one CPU.
> +		 */
> +		x86_pmu.amd_nb = 0;
> +	}
> +
>  	/* Events are common for all AMDs */
>  	memcpy(hw_cache_event_ids, amd_hw_cache_event_ids,
>  	       sizeof(hw_cache_event_ids));
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index ba6ef18528c9..46d2ece10a7b 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -608,6 +608,11 @@ struct x86_pmu {
>  	atomic_t	lbr_exclusive[x86_lbr_exclusive_max];
>  
>  	/*
> +	 * AMD bits
> +	 */
> +	unsigned int	amd_nb	: 1;
> +
> +	/*
>  	 * Extra registers for events
>  	 */
>  	struct extra_reg *extra_regs;
> 

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.