From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755527AbcCTNJi (ORCPT ); Sun, 20 Mar 2016 09:09:38 -0400 Received: from mail.skyhub.de ([78.46.96.112]:40637 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755426AbcCTNJ3 (ORCPT ); Sun, 20 Mar 2016 09:09:29 -0400 Date: Sun, 20 Mar 2016 14:09:26 +0100 From: Borislav Petkov To: Peter Zijlstra Cc: Thomas Gleixner , LKML , Ingo Molnar , aherrmann@suse.com, jencce.kernel@gmail.com, Rui Huang Subject: Re: [PATCH 2/3] x86/topology: Fix AMD core count Message-ID: <20160320130925.GC4230@pd.tnic> References: <20160318150345.146716865@infradead.org> <20160318150538.551407299@infradead.org> <20160320103946.GL6344@twins.programming.kicks-ass.net> <20160320110454.GB4230@pd.tnic> <20160320123225.GN6344@twins.programming.kicks-ass.net> <20160320124629.GY6375@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160320124629.GY6375@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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.