Topology evaluation is a complete disaster and impenetrable mess. It's
scattered all over the place with some vendor implementations doing early
evaluation and some not. The most horrific part is the permanent
overwriting of smt_max_siblings and __max_die_per_package, instead of
establishing them once on the boot CPU and validating the result on the
APs.
The goals are:
- One topology evaluation entry point
- Proper sharing of pointlessly duplicated code
- Proper structuring of the evaluation logic and preferences.
- Evaluating important system wide information only once on the boot CPU
- Making the 0xb/0x1f leaf parsing less convoluted and actually fixing
the short comings of leaf 0x1f evaluation.
Start to consolidate the topology evaluation code by providing the entry
points for the early boot CPU evaluation and for the final parsing on the
boot CPU and the APs.
Move the trivial pieces into that new code:
- The initialization of cpuinfo_x86::topo
- The evaluation of CPUID leaf 1, which presets topo::initial_apicid
- topo_apicid is set to topo::initial_apicid when invoked from early
boot. When invoked for the final evaluation on the boot CPU it reads
the actual APIC ID, which makes apic_get_initial_apicid() obsolete
once everything is converted over.
Provide a temporary helper function topo_converted() which shields off the
not yet converted CPU vendors from invoking code which would break them.
This shielding covers all vendor CPUs which support SMP, but not the
historical pure UP ones as they only need the topology info init and
eventually the initial APIC initialization.
Provide two new members in cpuinfo_x86::topo to store the maximum number of
SMT siblings and the number of dies per package and add them to the debugfs
readout. These two members will be used to populate this information on the
boot CPU and to validate the APs against it.
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/include/asm/topology.h | 19 +++
arch/x86/kernel/cpu/Makefile | 3
arch/x86/kernel/cpu/common.c | 23 +---
arch/x86/kernel/cpu/cpu.h | 6 +
arch/x86/kernel/cpu/debugfs.c | 37 ++++++
arch/x86/kernel/cpu/topology.h | 31 +++++
arch/x86/kernel/cpu/topology_common.c | 187 ++++++++++++++++++++++++++++++++++
7 files changed, 289 insertions(+), 17 deletions(-)
--- a/arch/x86/include/asm/topology.h
+++ b/arch/x86/include/asm/topology.h
@@ -102,6 +102,25 @@ static inline void setup_node_to_cpumask
#include <asm-generic/topology.h>
+/* Topology information */
+enum x86_topology_domains {
+ TOPO_SMT_DOMAIN,
+ TOPO_CORE_DOMAIN,
+ TOPO_MODULE_DOMAIN,
+ TOPO_TILE_DOMAIN,
+ TOPO_DIE_DOMAIN,
+ TOPO_PKG_DOMAIN,
+ TOPO_ROOT_DOMAIN,
+ TOPO_MAX_DOMAIN,
+};
+
+struct x86_topology_system {
+ unsigned int dom_shifts[TOPO_MAX_DOMAIN];
+ unsigned int dom_size[TOPO_MAX_DOMAIN];
+};
+
+extern struct x86_topology_system x86_topo_system;
+
extern const struct cpumask *cpu_coregroup_mask(int cpu);
extern const struct cpumask *cpu_clustergroup_mask(int cpu);
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -17,7 +17,8 @@ KMSAN_SANITIZE_common.o := n
# As above, instrumenting secondary CPU boot code causes boot hangs.
KCSAN_SANITIZE_common.o := n
-obj-y := cacheinfo.o scattered.o topology.o
+obj-y := cacheinfo.o scattered.o
+obj-y += topology_common.o topology.o
obj-y += common.o
obj-y += rdrand.o
obj-y += match.o
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1553,6 +1553,8 @@ static void __init early_identify_cpu(st
setup_force_cpu_cap(X86_FEATURE_CPUID);
cpu_parse_early_param();
+ cpu_init_topology(c);
+
if (this_cpu->c_early_init)
this_cpu->c_early_init(c);
@@ -1563,6 +1565,7 @@ static void __init early_identify_cpu(st
this_cpu->c_bsp_init(c);
} else {
setup_clear_cpu_cap(X86_FEATURE_CPUID);
+ cpu_init_topology(c);
}
setup_force_cpu_cap(X86_FEATURE_ALWAYS);
@@ -1708,18 +1711,6 @@ static void generic_identify(struct cpui
get_cpu_address_sizes(c);
- if (c->cpuid_level >= 0x00000001) {
- c->topo.initial_apicid = (cpuid_ebx(1) >> 24) & 0xFF;
-#ifdef CONFIG_X86_32
-# ifdef CONFIG_SMP
- c->topo.apicid = apic->phys_pkg_id(c->topo.initial_apicid, 0);
-# else
- c->topo.apicid = c->topo.initial_apicid;
-# endif
-#endif
- c->topo.pkg_id = c->topo.initial_apicid;
- }
-
get_model_name(c); /* Default name */
/*
@@ -1778,9 +1769,6 @@ static void identify_cpu(struct cpuinfo_
c->x86_model_id[0] = '\0'; /* Unset */
c->x86_max_cores = 1;
c->x86_coreid_bits = 0;
- c->topo.cu_id = 0xff;
- c->topo.llc_id = BAD_APICID;
- c->topo.l2c_id = BAD_APICID;
#ifdef CONFIG_X86_64
c->x86_clflush_size = 64;
c->x86_phys_bits = 36;
@@ -1799,6 +1787,8 @@ static void identify_cpu(struct cpuinfo_
generic_identify(c);
+ cpu_parse_topology(c);
+
if (this_cpu->c_identify)
this_cpu->c_identify(c);
@@ -1806,7 +1796,8 @@ static void identify_cpu(struct cpuinfo_
apply_forced_caps(c);
#ifdef CONFIG_X86_64
- c->topo.apicid = apic->phys_pkg_id(c->topo.initial_apicid, 0);
+ if (!topo_is_converted(c))
+ c->topo.apicid = apic->phys_pkg_id(c->topo.initial_apicid, 0);
#endif
/*
--- a/arch/x86/kernel/cpu/cpu.h
+++ b/arch/x86/kernel/cpu/cpu.h
@@ -2,6 +2,11 @@
#ifndef ARCH_X86_CPU_H
#define ARCH_X86_CPU_H
+#include <asm/cpu.h>
+#include <asm/topology.h>
+
+#include "topology.h"
+
/* attempt to consolidate cpu attributes */
struct cpu_dev {
const char *c_vendor;
@@ -95,4 +100,5 @@ static inline bool spectre_v2_in_eibrs_m
mode == SPECTRE_V2_EIBRS_RETPOLINE ||
mode == SPECTRE_V2_EIBRS_LFENCE;
}
+
#endif /* ARCH_X86_CPU_H */
--- a/arch/x86/kernel/cpu/debugfs.c
+++ b/arch/x86/kernel/cpu/debugfs.c
@@ -5,6 +5,8 @@
#include <asm/apic.h>
#include <asm/processor.h>
+#include "cpu.h"
+
static int cpu_debug_show(struct seq_file *m, void *p)
{
unsigned long cpu = (unsigned long)m->private;
@@ -42,12 +44,47 @@ static const struct file_operations dfs_
.release = single_release,
};
+static int dom_debug_show(struct seq_file *m, void *p)
+{
+ static const char *domain_names[TOPO_ROOT_DOMAIN] = {
+ [TOPO_SMT_DOMAIN] = "Thread",
+ [TOPO_CORE_DOMAIN] = "Core",
+ [TOPO_MODULE_DOMAIN] = "Module",
+ [TOPO_TILE_DOMAIN] = "Tile",
+ [TOPO_DIE_DOMAIN] = "Die",
+ [TOPO_PKG_DOMAIN] = "Package",
+ };
+ unsigned int dom, nthreads = 1;
+
+ for (dom = 0; dom < TOPO_ROOT_DOMAIN; dom++) {
+ nthreads *= x86_topo_system.dom_size[dom];
+ seq_printf(m, "domain: %-10s shift: %u dom_size: %5u max_threads: %5u\n",
+ domain_names[dom], x86_topo_system.dom_shifts[dom],
+ x86_topo_system.dom_size[dom], nthreads);
+ }
+ return 0;
+}
+
+static int dom_debug_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, dom_debug_show, inode->i_private);
+}
+
+static const struct file_operations dfs_dom_ops = {
+ .open = dom_debug_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
static __init int cpu_init_debugfs(void)
{
struct dentry *dir, *base = debugfs_create_dir("topo", arch_debugfs_dir);
unsigned long id;
char name[10];
+ debugfs_create_file("domains", 0444, base, NULL, &dfs_dom_ops);
+
dir = debugfs_create_dir("cpus", base);
for_each_possible_cpu(id) {
sprintf(name, "%lu", id);
--- /dev/null
+++ b/arch/x86/kernel/cpu/topology.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef ARCH_X86_TOPOLOGY_H
+#define ARCH_X86_TOPOLOGY_H
+
+struct topo_scan {
+ struct cpuinfo_x86 *c;
+ unsigned int dom_shifts[TOPO_MAX_DOMAIN];
+ unsigned int dom_ncpus[TOPO_MAX_DOMAIN];
+};
+
+bool topo_is_converted(struct cpuinfo_x86 *c);
+void cpu_init_topology(struct cpuinfo_x86 *c);
+void cpu_parse_topology(struct cpuinfo_x86 *c);
+void topology_set_dom(struct topo_scan *tscan, enum x86_topology_domains dom,
+ unsigned int shift, unsigned int ncpus);
+
+static inline u32 topo_shift_apicid(u32 apicid, enum x86_topology_domains dom)
+{
+ if (dom == TOPO_SMT_DOMAIN)
+ return apicid;
+ return apicid >> x86_topo_system.dom_shifts[dom - 1];
+}
+
+static inline u32 topo_relative_domain_id(u32 apicid, enum x86_topology_domains dom)
+{
+ if (dom != TOPO_SMT_DOMAIN)
+ apicid >>= x86_topo_system.dom_shifts[dom - 1];
+ return apicid & (x86_topo_system.dom_size[dom] - 1);
+}
+
+#endif /* ARCH_X86_TOPOLOGY_H */
--- /dev/null
+++ b/arch/x86/kernel/cpu/topology_common.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/cpu.h>
+
+#include <xen/xen.h>
+
+#include <asm/apic.h>
+#include <asm/processor.h>
+#include <asm/smp.h>
+
+#include "cpu.h"
+
+struct x86_topology_system x86_topo_system __ro_after_init;
+
+void topology_set_dom(struct topo_scan *tscan, enum x86_topology_domains dom,
+ unsigned int shift, unsigned int ncpus)
+{
+ tscan->dom_shifts[dom] = shift;
+ tscan->dom_ncpus[dom] = ncpus;
+
+ /* Propagate to the upper levels */
+ for (dom++; dom < TOPO_MAX_DOMAIN; dom++) {
+ tscan->dom_shifts[dom] = tscan->dom_shifts[dom - 1];
+ tscan->dom_ncpus[dom] = tscan->dom_ncpus[dom - 1];
+ }
+}
+
+bool topo_is_converted(struct cpuinfo_x86 *c)
+{
+ /* Temporary until everything is converted over. */
+ switch (boot_cpu_data.x86_vendor) {
+ case X86_VENDOR_AMD:
+ case X86_VENDOR_CENTAUR:
+ case X86_VENDOR_INTEL:
+ case X86_VENDOR_HYGON:
+ case X86_VENDOR_ZHAOXIN:
+ return false;
+ default:
+ /* Let all UP systems use the below */
+ return true;
+ }
+}
+
+static bool fake_topology(struct topo_scan *tscan)
+{
+ /*
+ * Preset the CORE level shift for CPUID less systems and XEN_PV,
+ * which has useless CPUID information.
+ */
+ topology_set_dom(tscan, TOPO_SMT_DOMAIN, 0, 1);
+ topology_set_dom(tscan, TOPO_CORE_DOMAIN, 1, 1);
+
+ return tscan->c->cpuid_level < 1 || xen_pv_domain();
+}
+
+static void parse_topology(struct topo_scan *tscan, bool early)
+{
+ const struct cpuinfo_topology topo_defaults = {
+ .cu_id = 0xff,
+ .llc_id = BAD_APICID,
+ .l2c_id = BAD_APICID,
+ };
+ struct cpuinfo_x86 *c = tscan->c;
+ struct {
+ u32 unused0 : 16,
+ nproc : 8,
+ apicid : 8;
+ } ebx;
+
+ c->topo = topo_defaults;
+
+ if (fake_topology(tscan))
+ return;
+
+ /* Preset Initial APIC ID from CPUID leaf 1 */
+ cpuid_leaf_reg(1, CPUID_EBX, &ebx);
+ c->topo.initial_apicid = ebx.apicid;
+
+ /*
+ * The initial invocation from early_identify_cpu() happens before
+ * the APIC is mapped or X2APIC enabled. For establishing the
+ * topology, that's not required. Use the initial APIC ID.
+ */
+ if (early)
+ c->topo.apicid = c->topo.initial_apicid;
+ else
+ c->topo.apicid = read_apic_id();
+
+ /* The above is sufficient for UP */
+ if (!IS_ENABLED(CONFIG_SMP))
+ return;
+}
+
+static void topo_set_ids(struct topo_scan *tscan)
+{
+ struct cpuinfo_x86 *c = tscan->c;
+ u32 apicid = c->topo.apicid;
+
+ c->topo.pkg_id = topo_shift_apicid(apicid, TOPO_PKG_DOMAIN);
+ c->topo.die_id = topo_shift_apicid(apicid, TOPO_DIE_DOMAIN);
+
+ /* Relative core ID */
+ c->topo.core_id = topo_relative_domain_id(apicid, TOPO_CORE_DOMAIN);
+}
+
+static void topo_set_max_cores(struct topo_scan *tscan)
+{
+ /*
+ * Bug compatible for now. This is broken on hybrid systems:
+ * 8 cores SMT + 8 cores w/o SMT
+ * tscan.dom_ncpus[TOPO_CORE_DOMAIN] = 24; 24 / 2 = 12 !!
+ *
+ * Cannot be fixed without further topology enumeration changes.
+ */
+ tscan->c->x86_max_cores = tscan->dom_ncpus[TOPO_CORE_DOMAIN] >>
+ x86_topo_system.dom_shifts[TOPO_SMT_DOMAIN];
+}
+
+void cpu_parse_topology(struct cpuinfo_x86 *c)
+{
+ unsigned int dom, cpu = smp_processor_id();
+ struct topo_scan tscan = { .c = c, };
+
+ parse_topology(&tscan, false);
+
+ if (!topo_is_converted(c))
+ return;
+
+ for (dom = TOPO_SMT_DOMAIN; dom < TOPO_MAX_DOMAIN; dom++) {
+ if (tscan.dom_shifts[dom] == x86_topo_system.dom_shifts[dom])
+ continue;
+ pr_err(FW_BUG "CPU%d: Topology domain %u shift %u != %u\n", cpu, dom,
+ tscan.dom_shifts[dom], x86_topo_system.dom_shifts[dom]);
+ }
+
+ /* Bug compatible with the existing parsers */
+ if (tscan.dom_ncpus[TOPO_SMT_DOMAIN] > smp_num_siblings) {
+ if (system_state == SYSTEM_BOOTING) {
+ pr_warn_once("CPU%d: SMT detected and enabled late\n", cpu);
+ smp_num_siblings = tscan.dom_ncpus[TOPO_SMT_DOMAIN];
+ } else {
+ pr_warn_once("CPU%d: SMT detected after init. Too late!\n", cpu);
+ }
+ }
+
+ topo_set_ids(&tscan);
+ topo_set_max_cores(&tscan);
+}
+
+void __init cpu_init_topology(struct cpuinfo_x86 *c)
+{
+ struct topo_scan tscan = { .c = c, };
+ unsigned int dom, sft;
+
+ parse_topology(&tscan, true);
+
+ if (!topo_is_converted(c))
+ return;
+
+ /* Copy the shift values and calculate the unit sizes. */
+ memcpy(x86_topo_system.dom_shifts, tscan.dom_shifts, sizeof(x86_topo_system.dom_shifts));
+
+ dom = TOPO_SMT_DOMAIN;
+ x86_topo_system.dom_size[dom] = 1U << x86_topo_system.dom_shifts[dom];
+
+ for (dom++; dom < TOPO_MAX_DOMAIN; dom++) {
+ sft = x86_topo_system.dom_shifts[dom] - x86_topo_system.dom_shifts[dom - 1];
+ x86_topo_system.dom_size[dom] = 1U << sft;
+ }
+
+ topo_set_ids(&tscan);
+ topo_set_max_cores(&tscan);
+
+ /*
+ * Bug compatible with the existing code. If the boot CPU does not
+ * have SMT this ends up with one sibling. This needs way deeper
+ * changes further down the road to get it right during early boot.
+ */
+ smp_num_siblings = tscan.dom_ncpus[TOPO_SMT_DOMAIN];
+
+ /*
+ * Neither it's clear whether there are as many dies as the APIC
+ * space indicating die level is. But assume that the actual number
+ * of CPUs gives a proper indication for now to stay bug compatible.
+ */
+ __max_die_per_package = tscan.dom_ncpus[TOPO_DIE_DOMAIN] /
+ tscan.dom_ncpus[TOPO_DIE_DOMAIN - 1];
+}
Hello Thomas,
On 8/2/2023 3:51 PM, Thomas Gleixner wrote:
>
> [..snip..]
>
> +static void topo_set_max_cores(struct topo_scan *tscan)
> +{
> + /*
> + * Bug compatible for now. This is broken on hybrid systems:
> + * 8 cores SMT + 8 cores w/o SMT
> + * tscan.dom_ncpus[TOPO_CORE_DOMAIN] = 24; 24 / 2 = 12 !!
> + *
> + * Cannot be fixed without further topology enumeration changes.
> + */
> + tscan->c->x86_max_cores = tscan->dom_ncpus[TOPO_CORE_DOMAIN] >>
> + x86_topo_system.dom_shifts[TOPO_SMT_DOMAIN];
> +}
>
In Documentation/arch/x86/topology.rst, "cpuinfo_x86.x86_max_cores" is
described as "The number of cores in a package". In which case,
shouldn't the above be:
tscan->c->x86_max_cores = tscan->dom_ncpus[TOPO_PKG_DOMAIN] >>
x86_topo_system.dom_shifts[TOPO_SMT_DOMAIN];
since, with extended topology, there could be other higher domains and
dom_ncpus[TOPO_CORE_DOMAIN] >> dom_shifts[TOPO_SMT_DOMAIN] should only
give number of cores within the next domain (TOPO_MODULE_DOMAIN).
Am I missing something here?
--
Thanks and Regards,
Prateek
Hello Thomas,
On 8/4/2023 1:58 PM, Thomas Gleixner wrote:
> On Fri, Aug 04 2023 at 13:44, K Prateek Nayak wrote:
>> On 8/2/2023 3:51 PM, Thomas Gleixner wrote:
>>>
>>> [..snip..]
>>>
>>> +static void topo_set_max_cores(struct topo_scan *tscan)
>>> +{
>>> + /*
>>> + * Bug compatible for now. This is broken on hybrid systems:
>>> + * 8 cores SMT + 8 cores w/o SMT
>>> + * tscan.dom_ncpus[TOPO_CORE_DOMAIN] = 24; 24 / 2 = 12 !!
>>> + *
>>> + * Cannot be fixed without further topology enumeration changes.
>>> + */
>>> + tscan->c->x86_max_cores = tscan->dom_ncpus[TOPO_CORE_DOMAIN] >>
>>> + x86_topo_system.dom_shifts[TOPO_SMT_DOMAIN];
>>> +}
>>>
>>
>> In Documentation/arch/x86/topology.rst, "cpuinfo_x86.x86_max_cores" is
>> described as "The number of cores in a package". In which case,
>> shouldn't the above be:
>>
>> tscan->c->x86_max_cores = tscan->dom_ncpus[TOPO_PKG_DOMAIN] >>
>> x86_topo_system.dom_shifts[TOPO_SMT_DOMAIN];
>>
>> since, with extended topology, there could be other higher domains and
>> dom_ncpus[TOPO_CORE_DOMAIN] >> dom_shifts[TOPO_SMT_DOMAIN] should only
>> give number of cores within the next domain (TOPO_MODULE_DOMAIN).
>
> You're right in principle.
>
>> Am I missing something here?
>
> The fact, that this is bug compatible. It's broken in several
> aspects. The real fix is in the next series, where this function goes
> away and actually uses real topology data to compute this.
>
> I could change this to be more "accurate" as you suggested, but that's
> not making much of a difference.
Ah! I see. Thank you for clarifying. I'll keep an eye out for the
next series.
--
Thanks and Regards,
Prateek
On Fri, Aug 04 2023 at 13:44, K Prateek Nayak wrote:
> On 8/2/2023 3:51 PM, Thomas Gleixner wrote:
>>
>> [..snip..]
>>
>> +static void topo_set_max_cores(struct topo_scan *tscan)
>> +{
>> + /*
>> + * Bug compatible for now. This is broken on hybrid systems:
>> + * 8 cores SMT + 8 cores w/o SMT
>> + * tscan.dom_ncpus[TOPO_CORE_DOMAIN] = 24; 24 / 2 = 12 !!
>> + *
>> + * Cannot be fixed without further topology enumeration changes.
>> + */
>> + tscan->c->x86_max_cores = tscan->dom_ncpus[TOPO_CORE_DOMAIN] >>
>> + x86_topo_system.dom_shifts[TOPO_SMT_DOMAIN];
>> +}
>>
>
> In Documentation/arch/x86/topology.rst, "cpuinfo_x86.x86_max_cores" is
> described as "The number of cores in a package". In which case,
> shouldn't the above be:
>
> tscan->c->x86_max_cores = tscan->dom_ncpus[TOPO_PKG_DOMAIN] >>
> x86_topo_system.dom_shifts[TOPO_SMT_DOMAIN];
>
> since, with extended topology, there could be other higher domains and
> dom_ncpus[TOPO_CORE_DOMAIN] >> dom_shifts[TOPO_SMT_DOMAIN] should only
> give number of cores within the next domain (TOPO_MODULE_DOMAIN).
You're right in principle.
> Am I missing something here?
The fact, that this is bug compatible. It's broken in several
aspects. The real fix is in the next series, where this function goes
away and actually uses real topology data to compute this.
I could change this to be more "accurate" as you suggested, but that's
not making much of a difference.
Thanks,
tglx
> +
> +static inline u32 topo_relative_domain_id(u32 apicid, enum
> x86_topology_domains dom)
> +{
> + if (dom != TOPO_SMT_DOMAIN)
> + apicid >>= x86_topo_system.dom_shifts[dom - 1];
> + return apicid & (x86_topo_system.dom_size[dom] - 1);
> +}
relative_domain_id() is used to get a unique id value within its next
higher level.
> +static void topo_set_ids(struct topo_scan *tscan)
> +{
> + struct cpuinfo_x86 *c = tscan->c;
> + u32 apicid = c->topo.apicid;
> +
> + c->topo.pkg_id = topo_shift_apicid(apicid, TOPO_PKG_DOMAIN);
> + c->topo.die_id = topo_shift_apicid(apicid, TOPO_DIE_DOMAIN);
> +
> + /* Relative core ID */
> + c->topo.core_id = topo_relative_domain_id(apicid,
> TOPO_CORE_DOMAIN);
My understanding is that, to ensure a package scope unique core_id,
rather than Module/Tile scope unique, what is really needed here is
something like,
apicid >>= x86_topo_system.dom_shifts[SMT];
c->topo.core_id = apicid & (x86_topo_system.dom_size[PACKAGE]
- 1);
I don't have chance to confirm this on a platform with Module level
yet, but will do soon.
thanks,
rui
On Sat, 2023-08-12 at 14:38 +0800, Zhang Rui wrote:
> > +
> > +static inline u32 topo_relative_domain_id(u32 apicid, enum
> > x86_topology_domains dom)
> > +{
> > + if (dom != TOPO_SMT_DOMAIN)
> > + apicid >>= x86_topo_system.dom_shifts[dom - 1];
> > + return apicid & (x86_topo_system.dom_size[dom] - 1);
> > +}
>
> relative_domain_id() is used to get a unique id value within its next
> higher level.
>
> > +static void topo_set_ids(struct topo_scan *tscan)
> > +{
> > + struct cpuinfo_x86 *c = tscan->c;
> > + u32 apicid = c->topo.apicid;
> > +
> > + c->topo.pkg_id = topo_shift_apicid(apicid,
> > TOPO_PKG_DOMAIN);
> > + c->topo.die_id = topo_shift_apicid(apicid,
> > TOPO_DIE_DOMAIN);
And die_id is also package scope unique before this patch series.
> > +
> > + /* Relative core ID */
> > + c->topo.core_id = topo_relative_domain_id(apicid,
> > TOPO_CORE_DOMAIN);
>
> My understanding is that, to ensure a package scope unique core_id,
> rather than Module/Tile scope unique, what is really needed here is
> something like,
> apicid >>= x86_topo_system.dom_shifts[SMT];
> c->topo.core_id = apicid & (x86_topo_system.dom_size[PACKAGE]
> - 1);
>
BTW, can we consider using system wide unique core_id instead?
There are a couple of advantages by using this.
CC Len, who can provide detailed justifications for this.
thanks,
rui
On Sat, 2023-08-12 at 06:41 +0000, Zhang, Rui wrote:
> > +
> > +static inline u32 topo_relative_domain_id(u32 apicid, enum
> > x86_topology_domains dom)
> > +{
> > + if (dom != TOPO_SMT_DOMAIN)
> > + apicid >>= x86_topo_system.dom_shifts[dom - 1];
> > + return apicid & (x86_topo_system.dom_size[dom] - 1);
> > +}
>
> relative_domain_id() is used to get a unique id value within its next
> higher level.
>
> > +static void topo_set_ids(struct topo_scan *tscan)
> > +{
> > + struct cpuinfo_x86 *c = tscan->c;
> > + u32 apicid = c->topo.apicid;
> > +
> > + c->topo.pkg_id = topo_shift_apicid(apicid,
> > TOPO_PKG_DOMAIN);
> > + c->topo.die_id = topo_shift_apicid(apicid,
> > TOPO_DIE_DOMAIN);
> > +
> > + /* Relative core ID */
> > + c->topo.core_id = topo_relative_domain_id(apicid,
> > TOPO_CORE_DOMAIN);
>
> My understanding is that, to ensure a package scope unique core_id,
> rather than Module/Tile scope unique, what is really needed here is
> something like,
> apicid >>= x86_topo_system.dom_shifts[SMT];
> c->topo.core_id = apicid & (x86_topo_system.dom_size[PACKAGE]
> - 1);
>
> I don't have chance to confirm this on a platform with Module level
> yet, but will do soon.
>
Tested on an AdlerLake-N platform, which has 2 Ecore Modules only.
[ 0.212526] CPU topo: Max. logical packages: 1
[ 0.212527] CPU topo: Max. logical dies: 1
[ 0.212528] CPU topo: Max. dies per package: 1
[ 0.212531] CPU topo: Max. threads per core: 1
[ 0.212532] CPU topo: Num. cores per package: 8
[ 0.212532] CPU topo: Num. threads per package: 8
[ 0.212532] CPU topo: Allowing 8 present CPUs plus 0 hotplug CPUs
[ 0.212535] CPU topo: Thread : 8
[ 0.212537] CPU topo: Core : 8
[ 0.212539] CPU topo: Module : 2
[ 0.212541] CPU topo: Tile : 1
[ 0.212543] CPU topo: Die : 1
[ 0.212545] CPU topo: Package : 1
This is all good, however,
# grep . /sys/devices/system/cpu/cpu*/topology/c*_id
/sys/devices/system/cpu/cpu0/topology/cluster_id:0
/sys/devices/system/cpu/cpu0/topology/core_id:0
/sys/devices/system/cpu/cpu1/topology/cluster_id:0
/sys/devices/system/cpu/cpu1/topology/core_id:1
/sys/devices/system/cpu/cpu2/topology/cluster_id:0
/sys/devices/system/cpu/cpu2/topology/core_id:2
/sys/devices/system/cpu/cpu3/topology/cluster_id:0
/sys/devices/system/cpu/cpu3/topology/core_id:3
/sys/devices/system/cpu/cpu4/topology/cluster_id:8
/sys/devices/system/cpu/cpu4/topology/core_id:0
/sys/devices/system/cpu/cpu5/topology/cluster_id:8
/sys/devices/system/cpu/cpu5/topology/core_id:1
/sys/devices/system/cpu/cpu6/topology/cluster_id:8
/sys/devices/system/cpu/cpu6/topology/core_id:2
/sys/devices/system/cpu/cpu7/topology/cluster_id:8
/sys/devices/system/cpu/cpu7/topology/core_id:3
The core_id is broken as it is Module scope unique only. To get package
scope unique core id, it should contain all bits up to package id bits.
thanks,
rui
On Sun, Aug 13 2023 at 13:30, Rui Zhang wrote:
> On Sat, 2023-08-12 at 06:41 +0000, Zhang, Rui wrote:
>> > +static inline u32 topo_relative_domain_id(u32 apicid, enum
>> > x86_topology_domains dom)
>> > +{
>> > + if (dom != TOPO_SMT_DOMAIN)
>> > + apicid >>= x86_topo_system.dom_shifts[dom - 1];
>> > + return apicid & (x86_topo_system.dom_size[dom] - 1);
>> > +}
>>
>> relative_domain_id() is used to get a unique id value within its next
>> higher level.
Correct.
>> > +static void topo_set_ids(struct topo_scan *tscan)
>> > +{
>> > + struct cpuinfo_x86 *c = tscan->c;
>> > + u32 apicid = c->topo.apicid;
>> > +
>> > + c->topo.pkg_id = topo_shift_apicid(apicid,
>> > TOPO_PKG_DOMAIN);
>> > + c->topo.die_id = topo_shift_apicid(apicid,
>> > TOPO_DIE_DOMAIN);
>> > +
>> > + /* Relative core ID */
>> > + c->topo.core_id = topo_relative_domain_id(apicid,
>> > TOPO_CORE_DOMAIN);
>>
>> My understanding is that, to ensure a package scope unique core_id,
>> rather than Module/Tile scope unique, what is really needed here is
>> something like,
>> apicid >>= x86_topo_system.dom_shifts[SMT];
>> c->topo.core_id = apicid & (x86_topo_system.dom_size[PACKAGE]
>> - 1);
Indeed.
> This is all good, however,
>
> # grep . /sys/devices/system/cpu/cpu*/topology/c*_id
> /sys/devices/system/cpu/cpu0/topology/cluster_id:0
> /sys/devices/system/cpu/cpu0/topology/core_id:0
> /sys/devices/system/cpu/cpu1/topology/cluster_id:0
> /sys/devices/system/cpu/cpu1/topology/core_id:1
> /sys/devices/system/cpu/cpu2/topology/cluster_id:0
> /sys/devices/system/cpu/cpu2/topology/core_id:2
> /sys/devices/system/cpu/cpu3/topology/cluster_id:0
> /sys/devices/system/cpu/cpu3/topology/core_id:3
> /sys/devices/system/cpu/cpu4/topology/cluster_id:8
> /sys/devices/system/cpu/cpu4/topology/core_id:0
> /sys/devices/system/cpu/cpu5/topology/cluster_id:8
> /sys/devices/system/cpu/cpu5/topology/core_id:1
> /sys/devices/system/cpu/cpu6/topology/cluster_id:8
> /sys/devices/system/cpu/cpu6/topology/core_id:2
> /sys/devices/system/cpu/cpu7/topology/cluster_id:8
> /sys/devices/system/cpu/cpu7/topology/core_id:3
>
> The core_id is broken as it is Module scope unique only. To get package
> scope unique core id, it should contain all bits up to package id bits.
Right. Let me correct that.
But aside of that we need to have a discussion urgently how we look at
these things. If relative, then relative to what.
Right now its how it happened to be, but there is not really a plan
behind all that.
Thanks,
tglx
On Sat, Aug 12 2023 at 08:00, Rui Zhang wrote:
> On Sat, 2023-08-12 at 14:38 +0800, Zhang Rui wrote:
> BTW, can we consider using system wide unique core_id instead?
>
> There are a couple of advantages by using this.
> CC Len, who can provide detailed justifications for this.
I have no problem with that. But as I said before we need a discussion
about the ID representation in general so it becomes a consistent view
at all levels.
The other thing to think about is whether we really need all these IDs
explicitly stored in cpu_info::topo... The vast majority of usage sites
is in some slow path (setup, cpu hotplug, proc, sysfs ...).
Thanks,
tglx
On Sun, Aug 13 2023 at 16:36, Thomas Gleixner wrote:
> On Sun, Aug 13 2023 at 13:30, Rui Zhang wrote:
>>> My understanding is that, to ensure a package scope unique core_id,
>>> rather than Module/Tile scope unique, what is really needed here is
>>> something like,
>>>>
>>> apicid >>= x86_topo_system.dom_shifts[SMT];
>>> c->topo.core_id = apicid & (x86_topo_system.dom_size[PACKAGE]
>>> - 1);
Actually it needs to be:
apicid &= (1U << x86_topo_system.dom_shifts[TOPO_PKG_DOMAIN]) - 1;
c->topo.core_id = apicid >> x86_topo_system.dom_shifts[TOPO_SMT_DOMAIN];
because otherwise you shift the lowest package ID bit into the result.
On Mon, 2023-08-14 at 08:20 +0200, Thomas Gleixner wrote:
> On Sun, Aug 13 2023 at 16:36, Thomas Gleixner wrote:
> > On Sun, Aug 13 2023 at 13:30, Rui Zhang wrote:
> > > > My understanding is that, to ensure a package scope unique
> > > > core_id,
> > > > rather than Module/Tile scope unique, what is really needed
> > > > here is
> > > > something like,
> > > > >
> > > > apicid >>= x86_topo_system.dom_shifts[SMT];
> > > > c->topo.core_id = apicid &
> > > > (x86_topo_system.dom_size[PACKAGE]
> > > > - 1);
>
> Actually it needs to be:
>
> apicid &= (1U <<
> x86_topo_system.dom_shifts[TOPO_PKG_DOMAIN]) - 1;
> c->topo.core_id = apicid >>
> x86_topo_system.dom_shifts[TOPO_SMT_DOMAIN];
>
> because otherwise you shift the lowest package ID bit into the
> result.
Agreed.
thanks,
rui
On Mon, 2023-08-14 at 08:26 +0200, Thomas Gleixner wrote:
> On Sat, Aug 12 2023 at 08:00, Rui Zhang wrote:
> > On Sat, 2023-08-12 at 14:38 +0800, Zhang Rui wrote:
> > BTW, can we consider using system wide unique core_id instead?
> >
> > There are a couple of advantages by using this.
> > CC Len, who can provide detailed justifications for this.
>
> I have no problem with that. But as I said before we need a
> discussion
> about the ID representation in general so it becomes a consistent
> view
> at all levels.
Agreed.
And I think Len will be back online and propose something soon and we
can start with that. :)
>
> The other thing to think about is whether we really need all these
> IDs
> explicitly stored in cpu_info::topo... The vast majority of usage
> sites
> is in some slow path (setup, cpu hotplug, proc, sysfs ...).
>
I don't have a strong preference here. With the new framework, this
info is really handy even if we don't cache it somewhere.
thanks,
rui