LinuxLists.cc - [PATCH 2/3] x86/topology: Fix AMD core count

[permalink] [raw]

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

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 <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Andreas Herrmann <[email protected]>
> Reported-by: Xiong Zhou <[email protected]>
> Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> ---
> 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.

2016-03-19 09:26:38

by Thomas Gleixner

[permalink] [raw]

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

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 <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Andreas Herrmann <[email protected]>
> Reported-by: Xiong Zhou <[email protected]>
> Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> ---
> 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

2016-03-19 15:57:11

[permalink] [raw]

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

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 <[email protected]>
> > Cc: Borislav Petkov <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Andreas Herrmann <[email protected]>
> > Reported-by: Xiong Zhou <[email protected]>
> > Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > Link: http://lkml.kernel.org/r/[email protected]
> > ---
> > 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.
--

2016-03-20 10:39:53

[permalink] [raw]

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

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.

2016-03-20 11:05:07

[permalink] [raw]

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

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.

2016-03-20 12:33:54

[permalink] [raw]

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

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.

2016-03-20 12:48:19

[permalink] [raw]

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

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;

2016-03-20 13:09:38

[permalink] [raw]

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

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.

2016-03-20 17:10:08

[permalink] [raw]

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

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.

2016-03-20 18:46:30

[permalink] [raw]

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

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.

2016-03-21 03:07:40

[permalink] [raw]

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

On Fri, Mar 18, 2016 at 05:41:01PM +0100, Borislav Petkov wrote:
> 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 <[email protected]>
> > Cc: Borislav Petkov <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Andreas Herrmann <[email protected]>
> > Reported-by: Xiong Zhou <[email protected]>
> > Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > Link: http://lkml.kernel.org/r/[email protected]
> > ---
> > 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.
>

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?

+ Sherry, for her awareness.

Thanks,
Rui

2016-03-21 03:46:29

[permalink] [raw]

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

On Mon, Mar 21, 2016 at 11:07:44AM +0800, Huang Rui wrote:
> On Fri, Mar 18, 2016 at 05:41:01PM +0100, Borislav Petkov wrote:
> > 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 <[email protected]>
> > > Cc: Borislav Petkov <[email protected]>
> > > Cc: Thomas Gleixner <[email protected]>
> > > Cc: Andreas Herrmann <[email protected]>
> > > Reported-by: Xiong Zhou <[email protected]>
> > > Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
> > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > > Link: http://lkml.kernel.org/r/[email protected]
> > > ---
> > > 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.
> >
>
> 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?
>
> + Sherry, for her awareness.
>

I quickly applied this patch on tip/master with on a fam15h machine.
The issue is still existed, only one core can be detected.

processor : 0
vendor_id : AuthenticAMD
cpu family : 21
model : 2
model name : AMD Opteron(tm) Processor 6386 SE
stepping : 0
microcode : 0x6000822
cpu MHz : 2792.882
cache size : 2048 KB
fpu : yes
fpu_exception : yes
cpuid level : 13
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb rdtscp lm constant_tsc rep_good nopl nonstop_tsc extd_apicid aperfmperf eagerfpu pni pclmulqdq monitor ssse3 fma cx16 sse4_1 sse4_2 popcnt aes xsave avx f16c lahf_lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch osvw ibs xop skinit wdt lwp fma4 tce nodeid_msr tbm topoext perfctr_core perfctr_nb cpb hw_pstate vmmcall bmi1 arat npt lbrv svm_lock nrip_save tsc_scale vmcb_clean flushbyasid decodeassists pausefilter pfthreshold
bugs : fxsave_leak sysret_ss_attrs
bogomips : 5585.76
TLB size : 1536 4K pages
clflush size : 64
cache_alignment : 64
address sizes : 48 bits physical, 48 bits virtual
power management: ts ttp tm 100mhzsteps hwpstate cpb eff_freq_ro

Architecture: x86_64
CPU op-mode(s): 32-bit, 64-bit
Byte Order: Little Endian
CPU(s): 1
On-line CPU(s) list: 0
Thread(s) per core: 1
Core(s) per socket: 1
Socket(s): 1
Vendor ID: AuthenticAMD
CPU family: 21
Model: 2
Stepping: 0
CPU MHz: 2792.882
BogoMIPS: 5585.76
Virtualization: AMD-V
L1d cache: 16K
L1i cache: 64K
L2 cache: 2048K
L3 cache: 6144K

Thanks,
Rui

2016-03-21 08:23:05

[permalink] [raw]

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

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.

2016-03-21 08:23:56

[permalink] [raw]

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

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.

2016-03-21 08:26:54

[permalink] [raw]

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

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.

2016-03-21 08:58:42

by Thomas Gleixner

[permalink] [raw]

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

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

2016-03-21 09:18:04

[permalink] [raw]

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

On Mon, Mar 21, 2016 at 09:26:43AM +0100, Borislav Petkov wrote:
> 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?
>

Yes, there are two sockets in this platform. 32 cores and 16 compute units.

> Are you saying, you have only one core in /proc/cpuinfo?
>
> Can you send full dmesg please?
>

Apology to bring a surprise. Our test machine didn't configure the
CONFIG_SMP just now because it was to reproduce another build issue
before...
So an incorrect "only one" core is detected, sorry. Please ignore previous
result. I will do more testing on other platforms.

Now I re-build system, please see the correct message:

ray@hr-ub:~/tip$ cat /sys/devices/system/cpu/cpu2/topology/thread_siblings_list
2-3
ray@hr-ub:~/tip$ cat /sys/devices/system/cpu/cpu2/topology/core_siblings_list
0-15

processor : 31
vendor_id : AuthenticAMD
cpu family : 21
model : 2
model name : AMD Opteron(tm) Processor 6386 SE
stepping : 0
microcode : 0x6000822
cpu MHz : 1400.000
cache size : 2048 KB
physical id : 1
siblings : 16
core id : 7
cpu cores : 8
apicid : 79
initial apicid : 47
fpu : yes
fpu_exception : yes
cpuid level : 13
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge
mca cmov pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext
fxsr_opt pdpe1gb rdtscp lm constant_tsc rep_good nopl nonstop_tsc
extd_apicid amd_dcm aperfmperf eagerfpu pni pclmulqdq monitor ssse3
fma cx16 sse4_1 sse4_2 popcnt aes xsave avx f16c lahf_lm cmp_legacy
svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch osvw ibs
xop skinit wdt lwp fma4 tce nodeid_msr tbm topoext perfctr_core
perfctr_nb cpb hw_pstate vmmcall bmi1 arat npt lbrv svm_lock nrip_save
tsc_scale vmcb_clean flushbyasid decodeassists pausefilter pfthreshold
bugs : fxsave_leak sysret_ss_attrs
bogomips : 5585.90
TLB size : 1536 4K pages
clflush size : 64
cache_alignment : 64
address sizes : 48 bits physical, 48 bits virtual
power management: ts ttp tm 100mhzsteps hwpstate cpb eff_freq_ro

Architecture: x86_64
CPU op-mode(s): 32-bit, 64-bit
Byte Order: Little Endian
CPU(s): 32
On-line CPU(s) list: 0-31
Thread(s) per core: 2
Core(s) per socket: 8
Socket(s): 2
NUMA node(s): 4
Vendor ID: AuthenticAMD
CPU family: 21
Model: 2
Stepping: 0
CPU MHz: 1400.000
BogoMIPS: 5585.90
Virtualization: AMD-V
L1d cache: 16K
L1i cache: 64K
L2 cache: 2048K
L3 cache: 6144K
NUMA node0 CPU(s): 0-7
NUMA node1 CPU(s): 8-15
NUMA node2 CPU(s): 16-23
NUMA node3 CPU(s): 24-31

Thanks,
Rui

2016-03-21 09:46:08

[permalink] [raw]

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

On Mon, Mar 21, 2016 at 09:21:29AM +0100, Peter Zijlstra wrote:
> 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.
>

OK, maybe, we would better add a comment to explain here. :-)

Thanks,
Rui

2016-03-21 10:05:13

[permalink] [raw]

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

On Mon, Mar 21, 2016 at 09:23:41AM +0100, Borislav Petkov wrote:
> 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. :-)
>

OK, actually, there was a topology bug on Carrzio before, the thread
number was detected as 1:

Before:

autotest@autotest-Gardenia88:~$ lscpu
Architecture: x86_64
CPU op-mode(s): 32-bit, 64-bit
Byte Order: Little Endian
CPU(s): 4
On-line CPU(s) list: 0-3
Thread(s) per core: 1
Core(s) per socket: 4
Socket(s): 1
NUMA node(s): 1
Vendor ID: AuthenticAMD
CPU family: 21
Model: 96
Model name: AMD Eng Sample: ZM1810C1Y4381_34/18/12/06_9874
Stepping: 0
CPU MHz: 1400.000
CPU max MHz: 1800.0000
CPU min MHz: 1400.0000
BogoMIPS: 3592.19
Virtualization: AMD-V
L1d cache: 32K
L1i cache: 64K
L2 cache: 1024K
NUMA node0 CPU(s): 0-3

Now:

ray@hr-ub:~/tip$ lscpu
Architecture: x86_64
CPU op-mode(s): 32-bit, 64-bit
Byte Order: Little Endian
CPU(s): 4
On-line CPU(s) list: 0-3
Thread(s) per core: 2
Core(s) per socket: 2
Socket(s): 1
NUMA node(s): 1
Vendor ID: AuthenticAMD
CPU family: 21
Model: 96
Stepping: 0
CPU MHz: 1400.000
BogoMIPS: 3592.19
Virtualization: AMD-V
L1d cache: 32K
L1i cache: 96K
L2 cache: 1024K
NUMA node0 CPU(s): 0-3

Looks better. I will test it on fam16h machine tomorrow, if it's OK,
will add my Test-by.

Thanks,
Rui

2016-03-21 10:23:35

[permalink] [raw]

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

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.

2016-03-21 13:57:41

[permalink] [raw]

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

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 <[email protected]>
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 <[email protected]>
---
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 <[email protected]>.
+
+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.

2016-03-22 07:56:49

[permalink] [raw]

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

On Fri, Mar 18, 2016 at 11:03:47PM +0800, 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 <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Andreas Herrmann <[email protected]>
> Reported-by: Xiong Zhou <[email protected]>
> Fixes: 1f12e32f4cd5 ("x86/topology: Create logical package id")
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>

I tested this patch both on fam15h and fam16h platforms, x86_max_cores
can report correct compute unit numbers. And
/sys/devices/system/cpu/cpuX/topology/thread_siblings_list
/sys/devices/system/cpu/cpuX/topology/core_siblings_list
are also right. Thanks.

Tested-by: Huang Rui <[email protected]>

> Link: http://lkml.kernel.org/r/[email protected]
> ---
> 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;
>
>

2016-03-22 08:11:42

by Hurwitz, Sherry

[permalink] [raw]

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

On 03/21/2016 08:57 AM, Borislav Petkov wrote:
> +- 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.
> -- hr
Boris, this documentation will help tremendously. 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.

2016-03-22 11:22:34