Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1360723 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/3] x86/topology: Fix AMD core count Peter Zijlstra <peterz@infradead.org> - 2016-03-18 16:10 +0100
    Re: [PATCH 2/3] x86/topology: Fix AMD core count Borislav Petkov <bp@alien8.de> - 2016-03-18 17:50 +0100
      Re: [PATCH 2/3] x86/topology: Fix AMD core count Borislav Petkov <bp@alien8.de> - 2016-03-21 09:30 +0100
      Re: [PATCH 2/3] x86/topology: Fix AMD core count Borislav Petkov <bp@alien8.de> - 2016-03-21 09:30 +0100
        Re: [PATCH 2/3] x86/topology: Fix AMD core count Borislav Petkov <bp@alien8.de> - 2016-03-21 11:30 +0100
      Re: [PATCH 2/3] x86/topology: Fix AMD core count Peter Zijlstra <peterz@infradead.org> - 2016-03-21 09:30 +0100
        Re: [PATCH 2/3] x86/topology: Fix AMD core count Borislav Petkov <bp@alien8.de> - 2016-03-21 15:00 +0100
          Re: [PATCH 2/3] x86/topology: Fix AMD core count Borislav Petkov <bp@alien8.de> - 2016-03-22 12:30 +0100
      Re: [PATCH 2/3] x86/topology: Fix AMD core count Thomas Gleixner <tglx@linutronix.de> - 2016-03-21 10:00 +0100
    Re: [PATCH 2/3] x86/topology: Fix AMD core count Thomas Gleixner <tglx@linutronix.de> - 2016-03-19 10:30 +0100
      Re: [PATCH 2/3] x86/topology: Fix AMD core count Borislav Petkov <bp@alien8.de> - 2016-03-19 17:00 +0100
      Re: [PATCH 2/3] x86/topology: Fix AMD core count Peter Zijlstra <peterz@infradead.org> - 2016-03-20 11:50 +0100
        Re: [PATCH 2/3] x86/topology: Fix AMD core count Borislav Petkov <bp@alien8.de> - 2016-03-20 12:10 +0100
          Re: [PATCH 2/3] x86/topology: Fix AMD core count Peter Zijlstra <peterz@infradead.org> - 2016-03-20 13:40 +0100
            Re: [PATCH 2/3] x86/topology: Fix AMD core count Peter Zijlstra <peterz@infradead.org> - 2016-03-20 13:50 +0100
              Re: [PATCH 2/3] x86/topology: Fix AMD core count Borislav Petkov <bp@alien8.de> - 2016-03-20 14:10 +0100
                Re: [PATCH 2/3] x86/topology: Fix AMD core count Peter Zijlstra <peterz@infradead.org> - 2016-03-20 18:20 +0100
                  Re: [PATCH 2/3] x86/topology: Fix AMD core count Borislav Petkov <bp@alien8.de> - 2016-03-20 19:50 +0100
              [tip:x86/urgent] perf/x86/amd: Cleanup Fam10h NB event constraints tip-bot for Peter Zijlstra <tipbot@zytor.com> - 2016-03-29 11:00 +0200

#1360723 — [PATCH 2/3] x86/topology: Fix AMD core count

It turns out AMD gets x86_max_cores wrong when there are compute
units.

The issue is that Linux assumes:

	nr_logical_cpus = nr_cores * nr_siblings

But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings
to 2 as well.

Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andreas Herrmann <aherrmann@suse.com>
Reported-by: Xiong Zhou <jencce.kernel@gmail.com>
Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20160317095220.GO6344@twins.programming.kicks-ass.net
---
 arch/x86/kernel/cpu/amd.c |    8 ++++----
 arch/x86/kernel/smpboot.c |   11 ++++++-----
 2 files changed, 10 insertions(+), 9 deletions(-)

--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -313,9 +313,9 @@ static void amd_get_topology(struct cpui
 		node_id = ecx & 7;
 
 		/* get compute unit information */
-		smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
+		c->x86_max_cores /= smp_num_siblings;
 		c->compute_unit_id = ebx & 0xff;
-		cores_per_cu += ((ebx >> 8) & 3);
 	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
 		u64 value;
 
@@ -331,8 +331,8 @@ static void amd_get_topology(struct cpui
 		u32 cus_per_node;
 
 		set_cpu_cap(c, X86_FEATURE_AMD_DCM);
-		cores_per_node = c->x86_max_cores / nodes_per_socket;
-		cus_per_node = cores_per_node / cores_per_cu;
+		cus_per_node = c->x86_max_cores / nodes_per_socket;
+		cores_per_node = cus_per_node * cores_per_cu;
 
 		/* store NodeID, use llc_shared_map to store sibling info */
 		per_cpu(cpu_llc_id, cpu) = node_id;

[toc] | [next] | [standalone]

#1360784

On Fri, Mar 18, 2016 at 04:03:47PM +0100, Peter Zijlstra wrote:
> It turns out AMD gets x86_max_cores wrong when there are compute
> units.
> 
> The issue is that Linux assumes:
> 
> 	nr_logical_cpus = nr_cores * nr_siblings
> 
> But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings
> to 2 as well.
> 
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andreas Herrmann <aherrmann@suse.com>
> Reported-by: Xiong Zhou <jencce.kernel@gmail.com>
> Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: http://lkml.kernel.org/r/20160317095220.GO6344@twins.programming.kicks-ass.net
> ---
>  arch/x86/kernel/cpu/amd.c |    8 ++++----
>  arch/x86/kernel/smpboot.c |   11 ++++++-----
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -313,9 +313,9 @@ static void amd_get_topology(struct cpui
>  		node_id = ecx & 7;
>  
>  		/* get compute unit information */
> -		smp_num_siblings = ((ebx >> 8) & 3) + 1;
> +		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
> +		c->x86_max_cores /= smp_num_siblings;
>  		c->compute_unit_id = ebx & 0xff;
> -		cores_per_cu += ((ebx >> 8) & 3);
>  	} else if (cpu_has(c, X86_FEATURE_NODEID_MSR)) {
>  		u64 value;
>  
> @@ -331,8 +331,8 @@ static void amd_get_topology(struct cpui
>  		u32 cus_per_node;
>  
>  		set_cpu_cap(c, X86_FEATURE_AMD_DCM);
> -		cores_per_node = c->x86_max_cores / nodes_per_socket;
> -		cus_per_node = cores_per_node / cores_per_cu;
> +		cus_per_node = c->x86_max_cores / nodes_per_socket;
> +		cores_per_node = cus_per_node * cores_per_cu;
>  
>  		/* store NodeID, use llc_shared_map to store sibling info */
>  		per_cpu(cpu_llc_id, cpu) = node_id;

Looks ok to me, however it probably would be prudent if AMD tested it on
a bunch of machines just to make sure we don't break anything else. I'm
thinking F15h and F16h, something big...

Rui, can you find some time to run this one please?

Look at before/after info in /proc/cpuinfo, topology in sysfs and dmesg
before and after might be useful too.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1361595

On Mon, Mar 21, 2016 at 11:46:19AM +0800, Huang Rui wrote:
> I quickly applied this patch on tip/master with on a fam15h machine.
> The issue is still existed, only one core can be detected.

Huh, what?

So that 6386 has 16 cores, according to wikipedia, that must be 8
compute units. Correct?

Are you saying, you have only one core in /proc/cpuinfo?

Can you send full dmesg please?

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1361600

On Mon, Mar 21, 2016 at 11:07:46AM +0800, Huang Rui wrote:
> OK, we will find some fam15h, fam16h platforms to verify it. Please
> wait for my feedback.
> 
> But I am confused with c->x86_max_cores /= smp_num_siblings, what is
> the real meaning of c->x86_max_cores here for AMD, the whole compute
> unit numbers per socket?

Yes, it is the cores and each core can contain two or more logical
threads. In AMD speak, that's the compute unit count. We read it in
detect_ht() from CPUID(1).EBX[23:16] which is LogicalProcessorCount,
i.e., CPUID(8000_0008).ECX[NC] + 1, i.e., the number of cores. And
"cores" in BKDG speak is the number of all cores in a processor which
are distributed across compute units....

That's why we divide by the number of siblings, i.e., the number of
cores in a CU, in AMD speak.

I know, it is confusing but once we're fine with the nomenclature, it'll
become as clear as day. :-)

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1361681

On Mon, Mar 21, 2016 at 06:05:16PM +0800, Huang Rui wrote:
> Looks better. I will test it on fam16h machine tomorrow, if it's OK,
> will add my Test-by.

Sure.

Thanks!

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1361603

On Mon, Mar 21, 2016 at 11:07:46AM +0800, Huang Rui wrote:
> > > The issue is that Linux assumes:
> > > 
> > > 	nr_logical_cpus = nr_cores * nr_siblings
> > > 
> > > But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings
> > > to 2 as well.

> But I am confused with c->x86_max_cores /= smp_num_siblings, what is
> the real meaning of c->x86_max_cores here for AMD, the whole compute
> unit numbers per socket?

Yes, with the whole Compute Unit being the Core, each logical CPU
becomes a Thread. This is the direct consequence of using the SMT
topology to model the CU thing.

[toc] | [prev] | [next] | [standalone]

#1361868

On Mon, Mar 21, 2016 at 05:46:12PM +0800, Huang Rui wrote:
> OK, maybe, we would better add a comment to explain here. :-)

Here's a start:

---
From: Borislav Petkov <bp@suse.de>
Date: Mon, 21 Mar 2016 14:53:05 +0100
Subject: [PATCH] x86/Documentation: Start documenting x86 topology

This should contain important aspects of how we represent the system
topology on x86. If people have questions about it and this file doesn't
answer it, then it must be updated.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 Documentation/x86/topology.txt | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/x86/topology.txt

diff --git a/Documentation/x86/topology.txt b/Documentation/x86/topology.txt
new file mode 100644
index 000000000000..828e953869d4
--- /dev/null
+++ b/Documentation/x86/topology.txt
@@ -0,0 +1,34 @@
+x86 Topology
+============
+
+This documents and clarifies the main aspects of x86 topology modelling
+and representation in the kernel. Update/change when doing changes to
+the respective code.
+
+Started by Borislav Petkov <bp@alien8.de>.
+
+The main aim of the topology facilities is to present adequate
+interfaces to code which needs to know/query/use the structure of the
+running system wrt threads, cores, nodes, etc.
+
+* cpuinfo_x86.x86_max_cores: the number of cores on a node, as reported
+by CPUID. Now, in order to accomodate both Intel and AMD, this variable
+means the following:
+
+- Intel: the number of cores in a processor. A core can have 1, 2 or
+more logical threads (hyperthreaded).
+
+- AMD: the number of cores in a processor. On a system where cores are
+clustered in groups of 2, 4 or more in compute units, this variable
+denotes the number of *compute units* on the node.
+
+In both cases, the number of scheduling threads is computed by doing:
+
+	x86_max_cores * smp_num_siblings
+
+This means:
+
+* smp_num_siblings: the number of siblings in a core. On AMD with
+compute units, this number is the number of compute unit siblings,
+i.e., compute unit cores in a single compute unit, according to their
+nomenclature.
-- 
2.7.3

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1362628

Hi Sherry,

On Tue, Mar 22, 2016 at 03:10:15AM -0500, Sherry Hurwitz wrote:
> Boris, this documentation will help tremendously.

Yeah, I'm collecting more stuff for it. If you feel like something else
should be explained there, holler.

> In just this line of
> code:
> 
> nr_local_cpus = nr_cores * nr_siblings
> 
> we have an Intel HT = AMD core = logical_cpu
> and core = AMD compute unit.
> 
> Anybody surprised there was a bug?
> 
> The surprising thing is that running lscpu on a 32 core 2 socket 6300
> Opteron system without
> the patches I get the same output as with the patches and it matches Ray's.
> I don't see an impact.

Yeah, so the logic is closer to an Intel core which can have 1 or more
threads and we did set x86_max_cores to the number of Bulldozer cores
(each of them in a pair forming a Compute Unit). And that's fine but
then we did set smp_num_siblings to 2.

In any case, I'm not seeing any issues here too but we wanted for you
guys to run those changes too before we apply them.

In any case, they will be in 4.6 and you could test tip/master from time
to time and scream if there's an issue.

Thanks!

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1361622

On Mon, 21 Mar 2016, Huang Rui wrote:
> I quickly applied this patch on tip/master with on a fam15h machine.

Can you use tip/x86/urgent please?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1361065

On Fri, 18 Mar 2016, Peter Zijlstra wrote:
> It turns out AMD gets x86_max_cores wrong when there are compute
> units.
> 
> The issue is that Linux assumes:
> 
> 	nr_logical_cpus = nr_cores * nr_siblings
> 
> But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings
> to 2 as well.
> 
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andreas Herrmann <aherrmann@suse.com>
> Reported-by: Xiong Zhou <jencce.kernel@gmail.com>
> Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: http://lkml.kernel.org/r/20160317095220.GO6344@twins.programming.kicks-ass.net
> ---
>  arch/x86/kernel/cpu/amd.c |    8 ++++----
>  arch/x86/kernel/smpboot.c |   11 ++++++-----
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -313,9 +313,9 @@ static void amd_get_topology(struct cpui
>  		node_id = ecx & 7;
>  
>  		/* get compute unit information */
> -		smp_num_siblings = ((ebx >> 8) & 3) + 1;
> +		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
> +		c->x86_max_cores /= smp_num_siblings;

Unfortunately that will break stuff in event/amd/core.c, ras/mce_amd_inj.c
which rely on the AMD interpretation of c->x86_max_cores.

I'm seriously grumpy about pointless inconsistent representations depending on
the CPU vendor. That's just lazy and sloppy hackery from the "works for me"
departement.

For now we'll get away with Peters workaround for the Intel HT mess, which is
a different kind of mental insanity invented by HW/BIOS people, but this
topology stuff needs to be made consistent ASAP.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1361129

On Sat, Mar 19, 2016 at 10:24:59AM +0100, Thomas Gleixner wrote:
> On Fri, 18 Mar 2016, Peter Zijlstra wrote:
> > It turns out AMD gets x86_max_cores wrong when there are compute
> > units.
> > 
> > The issue is that Linux assumes:
> > 
> > 	nr_logical_cpus = nr_cores * nr_siblings
> > 
> > But AMD reports its CU unit as 2 cores, but then sets num_smp_siblings
> > to 2 as well.
> > 
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Andreas Herrmann <aherrmann@suse.com>
> > Reported-by: Xiong Zhou <jencce.kernel@gmail.com>
> > Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Link: http://lkml.kernel.org/r/20160317095220.GO6344@twins.programming.kicks-ass.net
> > ---
> >  arch/x86/kernel/cpu/amd.c |    8 ++++----
> >  arch/x86/kernel/smpboot.c |   11 ++++++-----
> >  2 files changed, 10 insertions(+), 9 deletions(-)
> > 
> > --- a/arch/x86/kernel/cpu/amd.c
> > +++ b/arch/x86/kernel/cpu/amd.c
> > @@ -313,9 +313,9 @@ static void amd_get_topology(struct cpui
> >  		node_id = ecx & 7;
> >  
> >  		/* get compute unit information */
> > -		smp_num_siblings = ((ebx >> 8) & 3) + 1;
> > +		cores_per_cu = smp_num_siblings = ((ebx >> 8) & 3) + 1;
> > +		c->x86_max_cores /= smp_num_siblings;
> 
> Unfortunately that will break stuff in event/amd/core.c, ras/mce_amd_inj.c
> which rely on the AMD interpretation of c->x86_max_cores.

What is the events/amd/core.c check even mean? We alloc an amd_nb per
core but not per thread?

The mce_amd_inj.c thing we could fix probably like this:

---
diff --git a/arch/x86/ras/mce_amd_inj.c b/arch/x86/ras/mce_amd_inj.c
index 55d38cfa46c2..b5a917d943c8 100644
--- a/arch/x86/ras/mce_amd_inj.c
+++ b/arch/x86/ras/mce_amd_inj.c
@@ -203,12 +203,7 @@ static void trigger_thr_int(void *info)
 
 static u32 get_nbc_for_node(int node_id)
 {
-	struct cpuinfo_x86 *c = &boot_cpu_data;
-	u32 cores_per_node;
-
-	cores_per_node = c->x86_max_cores / amd_get_nodes_per_socket();
-
-	return cores_per_node * node_id;
+	return boot_cpu_data.x86_max_cores * node_id;
 }
 
 static void toggle_nb_mca_mst_cpu(u16 nid)
---

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1361238

On Sat, Mar 19, 2016 at 10:24:59AM +0100, Thomas Gleixner wrote:
> Unfortunately that will break stuff in event/amd/core.c, ras/mce_amd_inj.c
> which rely on the AMD interpretation of c->x86_max_cores.

So the AMD NB stuff in events/amd/core.c is only for Fam10, Fam15 got
its own uncore driver. (Fam10 had the uncore events through the same
counters as the core PMU with with 'fun' constraints).

And since Fam10 isn't affected by this change in x86_max_cores, this
_should_ work out, IF that NB code knows to switch off properly when not
required.

[toc] | [prev] | [next] | [standalone]

#1361241

On Sun, Mar 20, 2016 at 11:39:46AM +0100, Peter Zijlstra wrote:
> So the AMD NB stuff in events/amd/core.c is only for Fam10, Fam15 got
> its own uncore driver. (Fam10 had the uncore events through the same
> counters as the core PMU with with 'fun' constraints).
> 
> And since Fam10 isn't affected by this change in x86_max_cores, this
> _should_ work out, IF that NB code knows to switch off properly when not
> required.

But but, amd_core_pmu_init() is called on F15h too. It only gets the
different F15h event constraints and MSRs...

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1361263

On Sun, Mar 20, 2016 at 12:04:55PM +0100, Borislav Petkov wrote:
> On Sun, Mar 20, 2016 at 11:39:46AM +0100, Peter Zijlstra wrote:
> > So the AMD NB stuff in events/amd/core.c is only for Fam10, Fam15 got
> > its own uncore driver. (Fam10 had the uncore events through the same
> > counters as the core PMU with with 'fun' constraints).
> > 
> > And since Fam10 isn't affected by this change in x86_max_cores, this
> > _should_ work out, IF that NB code knows to switch off properly when not
> > required.
> 
> But but, amd_core_pmu_init() is called on F15h too. It only gets the
> different F15h event constraints and MSRs...

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.

[toc] | [prev] | [next] | [standalone]

#1361272

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.

---
 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)
 		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;
 };
 
 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;

[toc] | [prev] | [next] | [standalone]

#1361281

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.

[toc] | [prev] | [next] | [standalone]

#1361326

On Sun, Mar 20, 2016 at 02:09:26PM +0100, Borislav Petkov wrote:
> First a question about the big picture: why is amd/core.c even
> dealing with NB counters?

It it not, it is dealing with Fam10 NB events.

Fam10h doesn't have NB counters. Its NB events are on the same counters
as all the other events. Its just that they have constraints.

[toc] | [prev] | [next] | [standalone]

#1361380

On Sun, Mar 20, 2016 at 06:08:47PM +0100, Peter Zijlstra wrote:
> On Sun, Mar 20, 2016 at 02:09:26PM +0100, Borislav Petkov wrote:
> > First a question about the big picture: why is amd/core.c even
> > dealing with NB counters?
> 
> It it not, it is dealing with Fam10 NB events.
> 
> Fam10h doesn't have NB counters. Its NB events are on the same counters
> as all the other events. Its just that they have constraints.

See, and I was missing something: so there's the core x86_pmu and then
more PMUs get added with perf_pmu_register(). I.e., the uncore stuff, in
this case. Ok, makes sense...

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1366020 — [tip:x86/urgent] perf/x86/amd: Cleanup Fam10h NB event constraints

Commit-ID:  32b62f446827f696cc474a6d83cea93693c5ed49
Gitweb:     http://git.kernel.org/tip/32b62f446827f696cc474a6d83cea93693c5ed49
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Fri, 25 Mar 2016 15:52:35 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 29 Mar 2016 10:45:04 +0200

perf/x86/amd: Cleanup Fam10h NB event constraints

Avoid allocating the AMD NB event constraints data structure when not
needed. This gets rid of x86_max_cores usage and avoids allocating
this on AMD Core Perfctr supporting hardware (which has separate MSRs
for NB events).

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: aherrmann@suse.com
Cc: Rui Huang <ray.huang@amd.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: jencce.kernel@gmail.com
Link: http://lkml.kernel.org/r/20160320124629.GY6375@twins.programming.kicks-ass.net
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 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 049ada8d..86a9bec 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_constraints)
 		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_constraints)
 		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_constraints)
 		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_constraints	= 1,
 };
 
 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_constraints = 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_constraints = 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 ba6ef18..716d048 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_constraints : 1;
+
+	/*
 	 * Extra registers for events
 	 */
 	struct extra_reg *extra_regs;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web