detect_extended_topology() along with it's early() variant is a classic
example for duct tape engineering:
- It evaluates an array of subleafs with a boatload of local variables
for the relevant topology levels instead of using an array to save the
enumerated information and propagate it to the right level
- It has no boundary checks for subleafs
- It prevents updating the die_id with a crude workaround instead of
checking for leaf 0xb which does not provide die information.
- It's broken vs. the number of dies evaluation as it uses:
num_processors[DIE_LEVEL] / num_processors[CORE_LEVEL]
which "works" only correctly if there is none of the intermediate
topology levels (MODULE/TILE) enumerated.
There is zero value in trying to "fix" that code as the only proper fix is
to rewrite it from scratch.
Implement a sane parser with proper code documentation, which will be used
for the consolidated topology evaluation in the next step.
Signed-off-by: Thomas Gleixner <[email protected]>
---
V2: Fixed up the comment alignment for registers - Peterz
---
arch/x86/kernel/cpu/Makefile | 2
arch/x86/kernel/cpu/topology.h | 12 +++
arch/x86/kernel/cpu/topology_ext.c | 136 +++++++++++++++++++++++++++++++++++++
3 files changed, 149 insertions(+), 1 deletion(-)
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -18,7 +18,7 @@ KMSAN_SANITIZE_common.o := n
KCSAN_SANITIZE_common.o := n
obj-y := cacheinfo.o scattered.o
-obj-y += topology_common.o topology.o
+obj-y += topology_common.o topology_ext.o topology.o
obj-y += common.o
obj-y += rdrand.o
obj-y += match.o
--- a/arch/x86/kernel/cpu/topology.h
+++ b/arch/x86/kernel/cpu/topology.h
@@ -16,6 +16,7 @@ void cpu_init_topology(struct cpuinfo_x8
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);
+bool cpu_parse_topology_ext(struct topo_scan *tscan);
static inline u32 topo_shift_apicid(u32 apicid, enum x86_topology_domains dom)
{
@@ -31,4 +32,15 @@ static inline u32 topo_relative_domain_i
return apicid & (x86_topo_system.dom_size[dom] - 1);
}
+/*
+ * Update a domain level after the fact without propagating. Used to fixup
+ * broken CPUID enumerations.
+ */
+static inline void topology_update_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;
+}
+
#endif /* ARCH_X86_TOPOLOGY_H */
--- /dev/null
+++ b/arch/x86/kernel/cpu/topology_ext.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/cpu.h>
+
+#include <asm/apic.h>
+#include <asm/memtype.h>
+#include <asm/processor.h>
+
+#include "cpu.h"
+
+enum topo_types {
+ INVALID_TYPE = 0,
+ SMT_TYPE = 1,
+ CORE_TYPE = 2,
+ MODULE_TYPE = 3,
+ TILE_TYPE = 4,
+ DIE_TYPE = 5,
+ DIEGRP_TYPE = 6,
+ MAX_TYPE = 7,
+};
+
+/*
+ * Use a lookup table for the case that there are future types > 6 which
+ * describe an intermediate domain level which does not exist today.
+ *
+ * A table will also be handy to parse the new AMD 0x80000026 leaf which
+ * has defined different domain types, but otherwise uses the same layout
+ * with some of the reserved bits used for new information.
+ */
+static const unsigned int topo_domain_map[MAX_TYPE] = {
+ [SMT_TYPE] = TOPO_SMT_DOMAIN,
+ [CORE_TYPE] = TOPO_CORE_DOMAIN,
+ [MODULE_TYPE] = TOPO_MODULE_DOMAIN,
+ [TILE_TYPE] = TOPO_TILE_DOMAIN,
+ [DIE_TYPE] = TOPO_DIE_DOMAIN,
+ [DIEGRP_TYPE] = TOPO_PKG_DOMAIN,
+};
+
+static inline bool topo_subleaf(struct topo_scan *tscan, u32 leaf, u32 subleaf)
+{
+ unsigned int dom, maxtype = leaf == 0xb ? CORE_TYPE + 1 : MAX_TYPE;
+ struct {
+ // eax
+ u32 x2apic_shift : 5, // Number of bits to shift APIC ID right
+ // for the topology ID at the next level
+ __rsvd0 : 27; // Reserved
+ // ebx
+ u32 num_processors : 16, // Number of processors at current level
+ __rsvd1 : 16; // Reserved
+ // ecx
+ u32 level : 8, // Current topology level. Same as sub leaf number
+ type : 8, // Level type. If 0, invalid
+ __rsvd2 : 16; // Reserved
+ // edx
+ u32 x2apic_id : 32; // X2APIC ID of the current logical processor
+ } sl;
+
+ cpuid_subleaf(leaf, subleaf, &sl);
+
+ if (!sl.num_processors || sl.type == INVALID_TYPE)
+ return false;
+
+ if (sl.type >= maxtype) {
+ /*
+ * As the subleafs are ordered in domain level order, this
+ * could be recovered in theory by propagating the
+ * information at the last parsed level.
+ *
+ * But if the infinite wisdom of hardware folks decides to
+ * create a new domain type between CORE and MODULE or DIE
+ * and DIEGRP, then that would overwrite the CORE or DIE
+ * information.
+ *
+ * It really would have been too obvious to make the domain
+ * type space sparse and leave a few reserved types between
+ * the points which might change instead of forcing
+ * software to either create a monstrosity of workarounds
+ * or just being up the creek without a paddle.
+ *
+ * Refuse to implement monstrosity, emit an error and try
+ * to survive.
+ */
+ pr_err_once("Topology: leaf 0x%x:%d Unknown domain type %u\n",
+ leaf, subleaf, sl.type);
+ return true;
+ }
+
+ dom = topo_domain_map[sl.type];
+ if (!dom) {
+ tscan->c->topo.initial_apicid = sl.x2apic_id;
+ } else if (tscan->c->topo.initial_apicid != sl.x2apic_id) {
+ pr_warn_once(FW_BUG "CPUID leaf 0x%x subleaf %d APIC ID mismatch %x != %x\n",
+ leaf, subleaf, tscan->c->topo.initial_apicid, sl.x2apic_id);
+ }
+
+ topology_set_dom(tscan, dom, sl.x2apic_shift, sl.num_processors);
+ return true;
+}
+
+static bool parse_topology_leaf(struct topo_scan *tscan, u32 leaf)
+{
+ u32 subleaf;
+
+ if (tscan->c->cpuid_level < leaf)
+ return false;
+
+ /* Read all available subleafs and populate the levels */
+ for (subleaf = 0; topo_subleaf(tscan, leaf, subleaf); subleaf++);
+
+ /* If subleaf 0 failed to parse, give up */
+ if (!subleaf)
+ return false;
+
+ /*
+ * There are machines in the wild which have shift 0 in the subleaf
+ * 0, but advertise 2 logical processors at that level. They are
+ * truly SMT.
+ */
+ if (!tscan->dom_shifts[TOPO_SMT_DOMAIN] && tscan->dom_ncpus[TOPO_SMT_DOMAIN] > 1) {
+ unsigned int sft = get_count_order(tscan->dom_ncpus[TOPO_SMT_DOMAIN]);
+
+ pr_warn_once(FW_BUG "CPUID leaf 0x%x subleaf 0 has shift level 0 but %u CPUs\n",
+ leaf, tscan->dom_ncpus[TOPO_SMT_DOMAIN]);
+ topology_update_dom(tscan, TOPO_SMT_DOMAIN, sft, tscan->dom_ncpus[TOPO_SMT_DOMAIN]);
+ }
+
+ set_cpu_cap(tscan->c, X86_FEATURE_XTOPOLOGY);
+ return true;
+}
+
+bool cpu_parse_topology_ext(struct topo_scan *tscan)
+{
+ /* Try lead 0x1F first. If not available try leaf 0x0b */
+ if (parse_topology_leaf(tscan, 0x1f))
+ return true;
+ return parse_topology_leaf(tscan, 0x0b);
+}
Hi, Thomas,
On Wed, 2023-08-02 at 12:21 +0200, Thomas Gleixner wrote:
> detect_extended_topology() along with it's early() variant is a
> classic
> example for duct tape engineering:
>
> - It evaluates an array of subleafs with a boatload of local
> variables
> for the relevant topology levels instead of using an array to
> save the
> enumerated information and propagate it to the right level
>
> - It has no boundary checks for subleafs
>
> - It prevents updating the die_id with a crude workaround instead
> of
> checking for leaf 0xb which does not provide die information.
>
> - It's broken vs. the number of dies evaluation as it uses:
>
> num_processors[DIE_LEVEL] / num_processors[CORE_LEVEL]
>
> which "works" only correctly if there is none of the intermediate
> topology levels (MODULE/TILE) enumerated.
>
> There is zero value in trying to "fix" that code as the only proper
> fix is
> to rewrite it from scratch.
>
> Implement a sane parser with proper code documentation, which will be
> used
> for the consolidated topology evaluation in the next step.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> V2: Fixed up the comment alignment for registers - Peterz
> ---
> arch/x86/kernel/cpu/Makefile | 2
> arch/x86/kernel/cpu/topology.h | 12 +++
> arch/x86/kernel/cpu/topology_ext.c | 136
> +++++++++++++++++++++++++++++++++++++
> 3 files changed, 149 insertions(+), 1 deletion(-)
>
> --- a/arch/x86/kernel/cpu/Makefile
> +++ b/arch/x86/kernel/cpu/Makefile
> @@ -18,7 +18,7 @@ KMSAN_SANITIZE_common.o := n
> KCSAN_SANITIZE_common.o := n
>
> obj-y := cacheinfo.o scattered.o
> -obj-y += topology_common.o topology.o
> +obj-y += topology_common.o topology_ext.o
> topology.o
> obj-y += common.o
> obj-y += rdrand.o
> obj-y += match.o
> --- a/arch/x86/kernel/cpu/topology.h
> +++ b/arch/x86/kernel/cpu/topology.h
> @@ -16,6 +16,7 @@ void cpu_init_topology(struct cpuinfo_x8
> 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);
> +bool cpu_parse_topology_ext(struct topo_scan *tscan);
>
> static inline u32 topo_shift_apicid(u32 apicid, enum
> x86_topology_domains dom)
> {
> @@ -31,4 +32,15 @@ static inline u32 topo_relative_domain_i
> return apicid & (x86_topo_system.dom_size[dom] - 1);
> }
>
> +/*
> + * Update a domain level after the fact without propagating. Used to
> fixup
> + * broken CPUID enumerations.
> + */
> +static inline void topology_update_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;
> +}
> +
> #endif /* ARCH_X86_TOPOLOGY_H */
> --- /dev/null
> +++ b/arch/x86/kernel/cpu/topology_ext.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/cpu.h>
> +
> +#include <asm/apic.h>
> +#include <asm/memtype.h>
> +#include <asm/processor.h>
> +
> +#include "cpu.h"
> +
> +enum topo_types {
> + INVALID_TYPE = 0,
> + SMT_TYPE = 1,
> + CORE_TYPE = 2,
> + MODULE_TYPE = 3,
> + TILE_TYPE = 4,
> + DIE_TYPE = 5,
> + DIEGRP_TYPE = 6,
> + MAX_TYPE = 7,
> +};
> +
> +/*
> + * Use a lookup table for the case that there are future types > 6
> which
> + * describe an intermediate domain level which does not exist today.
> + *
> + * A table will also be handy to parse the new AMD 0x80000026 leaf
> which
> + * has defined different domain types, but otherwise uses the same
> layout
> + * with some of the reserved bits used for new information.
> + */
> +static const unsigned int topo_domain_map[MAX_TYPE] = {
> + [SMT_TYPE] = TOPO_SMT_DOMAIN,
> + [CORE_TYPE] = TOPO_CORE_DOMAIN,
> + [MODULE_TYPE] = TOPO_MODULE_DOMAIN,
> + [TILE_TYPE] = TOPO_TILE_DOMAIN,
> + [DIE_TYPE] = TOPO_DIE_DOMAIN,
> + [DIEGRP_TYPE] = TOPO_PKG_DOMAIN,
May I know why DIEGRP_TYPE is mapped to TOPO_PKG_DOMAIN?
> +};
> +
> +static inline bool topo_subleaf(struct topo_scan *tscan, u32 leaf,
> u32 subleaf)
> +{
> + unsigned int dom, maxtype = leaf == 0xb ? CORE_TYPE + 1 :
> MAX_TYPE;
> + struct {
> + // eax
> + u32 x2apic_shift : 5, // Number of bits to
> shift APIC ID right
> + // for the topology ID
> at the next level
> + __rsvd0 : 27; // Reserved
> + // ebx
> + u32 num_processors : 16, // Number of processors
> at current level
> + __rsvd1 : 16; // Reserved
> + // ecx
> + u32 level : 8, // Current topology
> level. Same as sub leaf number
> + type : 8, // Level type. If 0,
> invalid
> + __rsvd2 : 16; // Reserved
> + // edx
> + u32 x2apic_id : 32; // X2APIC ID of the
> current logical processor
> + } sl;
> +
> + cpuid_subleaf(leaf, subleaf, &sl);
> +
> + if (!sl.num_processors || sl.type == INVALID_TYPE)
> + return false;
> +
> + if (sl.type >= maxtype) {
It is still legal to have sparse type value in the future, and then
this check will break.
IMO, it is better to use a function to convert type to domain, and
check for unknown domain here, say, something like
diff --git a/arch/x86/kernel/cpu/topology_ext.c
b/arch/x86/kernel/cpu/topology_ext.c
index 5ddc5d24435e..7720a7bc7478 100644
--- a/arch/x86/kernel/cpu/topology_ext.c
+++ b/arch/x86/kernel/cpu/topology_ext.c
@@ -26,14 +26,27 @@ enum topo_types {
* has defined different domain types, but otherwise uses the same
layout
* with some of the reserved bits used for new information.
*/
-static const unsigned int topo_domain_map[MAX_TYPE] = {
- [SMT_TYPE] = TOPO_SMT_DOMAIN,
- [CORE_TYPE] = TOPO_CORE_DOMAIN,
- [MODULE_TYPE] = TOPO_MODULE_DOMAIN,
- [TILE_TYPE] = TOPO_TILE_DOMAIN,
- [DIE_TYPE] = TOPO_DIE_DOMAIN,
- [DIEGRP_TYPE] = TOPO_PKG_DOMAIN,
-};
+
+static enum x86_topology_domains topo_type_to_domain(int type)
+{
+ switch (type) {
+ case SMT_TYPE:
+ return TOPO_SMT_DOMAIN;
+ case CORE_TYPE:
+ return TOPO_CORE_DOMAIN;
+ case MODULE_TYPE:
+ return TOPO_MODULE_DOMAIN;
+ case TILE_TYPE:
+ return TOPO_TILE_DOMAIN;
+ case DIE_TYPE:
+ return TOPO_DIE_DOMAIN;
+ case DIEGRP_TYPE:
+ return TOPO_PKG_DOMAIN;
+ default:
+ return TOPO_MAX_DOMAIN;
+ }
+
+}
static inline bool topo_subleaf(struct topo_scan *tscan, u32 leaf, u32
subleaf)
{
@@ -59,7 +72,8 @@ static inline bool topo_subleaf(struct topo_scan
*tscan, u32 leaf, u32 subleaf)
if (!sl.num_processors || sl.type == INVALID_TYPE)
return false;
- if (sl.type >= maxtype) {
+ dom = topo_type_to_domain(sl.type);
+ if (dom == TOPO_MAX_DOMAIN) {
/*
* As the subleafs are ordered in domain level order,
this
* could be recovered in theory by propagating the
@@ -84,7 +98,6 @@ static inline bool topo_subleaf(struct topo_scan
*tscan, u32 leaf, u32 subleaf)
return true;
}
- dom = topo_domain_map[sl.type];
if (!dom) {
tscan->c->topo.initial_apicid = sl.x2apic_id;
} else if (tscan->c->topo.initial_apicid != sl.x2apic_id) {
> + /*
> + * As the subleafs are ordered in domain level order,
> this
> + * could be recovered in theory by propagating the
> + * information at the last parsed level.
> + *
> + * But if the infinite wisdom of hardware folks
> decides to
> + * create a new domain type between CORE and MODULE
> or DIE
> + * and DIEGRP, then that would overwrite the CORE or
> DIE
> + * information.
Sorry that I'm confused here.
Say, we have CORE, FOO, MODULE, then the subleave of FOO must be higher
than CORE but lower than MODULE.
so we parse CORE first and propagates the info to FOO/MODULE, and then
parse FOO and propagate to MODULE, and parse MODULE in the end.
How could we overwrite the info of a lower level?
> + *
> + * It really would have been too obvious to make the
> domain
> + * type space sparse and leave a few reserved types
> between
> + * the points which might change instead of forcing
> + * software to either create a monstrosity of
> workarounds
> + * or just being up the creek without a paddle.
Agreed.
with sparse type space, we know the relationship between different
types, without knowing what the type really means.
> + *
> + * Refuse to implement monstrosity, emit an error and try
> + * to survive.
> + */
> + pr_err_once("Topology: leaf 0x%x:%d Unknown domain type %u\n",
> + leaf, subleaf, sl.type);
> + return true;
Don't want to be TLDR, I can think of a couple cases that breaks Linux
in different ways if we ignore the cpu topology info of an unknown
level.
So I just want to understand the strategy here, does this mean that
we're not looking for a future proof solution, and instead we are
planning to take future updates (patch enum topo_types/enum
x86_topology_domains/topo_domain_map) whenever a new level is invented?
TBH, I'm still thinking of a future proof proposal here.
currently, Linux only cares about pkg_id/core_id/die_id, and the
relationship between these three levels.
1. for package id: pkg_id_low = FOO.x2apic_shift (FOO is the highest
enumerated level, no matter its type is known or not)
2. for core_id: as SMT level is always enumerated, core_id_low =
SMT.x2apic_shift, core_id_high = pkg_id_low - 1;
3. for die_id: Make Linux Die *OPTIONAL*.
when DIE is enumerated via CPUID.1F, die_id_low = FOO.x2apic_shift
(FOO is the next enumerated lower level of DIE, no matter its type is
known or not), die_id_high = pkg_id_low - 1;
when DIE is not enumerated via CPUID.1F, then Linux die does not
exist, adjust the die related topology information, say, die_id = -1,
topology_max_dies_per_package = 0, etc, and don't expose die sysfs I/F.
With this, we can guarantee that all the available topology information
are always valid, even when running on future platforms.
what do you think?
thanks,
rui
On Sat, Aug 12 2023 at 08:21, Rui Zhang wrote:
> On Wed, 2023-08-02 at 12:21 +0200, Thomas Gleixner wrote:
>> +
>> +/*
>> + * Use a lookup table for the case that there are future types > 6
>> which
>> + * describe an intermediate domain level which does not exist today.
>> + *
>> + * A table will also be handy to parse the new AMD 0x80000026 leaf
>> which
>> + * has defined different domain types, but otherwise uses the same
>> layout
>> + * with some of the reserved bits used for new information.
>> + */
>> +static const unsigned int topo_domain_map[MAX_TYPE] = {
>> + [SMT_TYPE] = TOPO_SMT_DOMAIN,
>> + [CORE_TYPE] = TOPO_CORE_DOMAIN,
>> + [MODULE_TYPE] = TOPO_MODULE_DOMAIN,
>> + [TILE_TYPE] = TOPO_TILE_DOMAIN,
>> + [DIE_TYPE] = TOPO_DIE_DOMAIN,
>> + [DIEGRP_TYPE] = TOPO_PKG_DOMAIN,
>
> May I know why DIEGRP_TYPE is mapped to TOPO_PKG_DOMAIN?
Where else should it go? It's the topmost, no? But diegrp is a
terminoligy which is not used in the kernel
>> +
>> + if (sl.type >= maxtype) {
>
> It is still legal to have sparse type value in the future, and then
> this check will break.
> IMO, it is better to use a function to convert type to domain, and
> check for unknown domain here, say, something like
Why? If somewhere in the future Intel decides to add UBER_TILE_TYPE,
then this will be a type larger than DIEGRP_TYPE. maxtype will then
cover the whole thing and the table will map it to the right place.
Even if in their infinite wisdom the HW folks decide to make a gap, then
the table can handle it simply by putting an invalid value into the gap
and checking for that.
Serioulsy we don't need a switch case for that.
>> + /*
>> + * As the subleafs are ordered in domain level order,
>> this
>> + * could be recovered in theory by propagating the
>> + * information at the last parsed level.
>> + *
>> + * But if the infinite wisdom of hardware folks
>> decides to
>> + * create a new domain type between CORE and MODULE
>> or DIE
>> + * and DIEGRP, then that would overwrite the CORE or
>> DIE
>> + * information.
>
> Sorry that I'm confused here.
>
> Say, we have CORE, FOO, MODULE, then the subleave of FOO must be higher
> than CORE but lower than MODULE.
> so we parse CORE first and propagates the info to FOO/MODULE, and then
> parse FOO and propagate to MODULE, and parse MODULE in the end.
> How could we overwrite the info of a lower level?
We don't know about this new thing yet. So where should we propagate to?
We could say, last was core so we stick the new thing into module, but
do we know that's correct? Do we know there is a module actually. Let me
rephrase that comment.
>> + *
>> + * Refuse to implement monstrosity, emit an error and try
>> + * to survive.
>> + */
>> + pr_err_once("Topology: leaf 0x%x:%d Unknown domain type %u\n",
>> + leaf, subleaf, sl.type);
>> + return true;
>
> Don't want to be TLDR, I can think of a couple cases that breaks Linux
> in different ways if we ignore the cpu topology info of an unknown
> level.
Come on. If Intel manages it to create a new level then it's not rocket
science to integrate support for that a long time before actual silicon
ships. So what will break? The machines of people who use ancient
kernels on modern hardware? They can keep the pieces.
> So I just want to understand the strategy here, does this mean that
> we're not looking for a future proof solution, and instead we are
> planning to take future updates (patch enum topo_types/enum
> x86_topology_domains/topo_domain_map) whenever a new level is
> invented?
You need that anyway, no?
> With this, we can guarantee that all the available topology information
> are always valid, even when running on future platforms.
I know that it can be made work, but is it worth the extra effort? I
don't think so.
Thanks,
tglx
On Sat, Aug 12 2023 at 22:04, Thomas Gleixner wrote:
> On Sat, Aug 12 2023 at 08:21, Rui Zhang wrote:
>> With this, we can guarantee that all the available topology information
>> are always valid, even when running on future platforms.
>
> I know that it can be made work, but is it worth the extra effort? I
> don't think so.
So I thought more about it. For intermediate levels, i.e. something
which is squeezed between two existing levels, this works by some
definition of works.
I.e. the example where we have UBER_TILE between TILE and DIE, then we'd
set and propagate the UBER_TILE entry into the DIE slot and then
overwrite it again, if there is a DIE entry too.
Where it becomes interesting is when the unknown level is past DIEGRP,
e.g. DIEGRP_CONGLOMORATE then we'd need to overwrite the DIEGRP level,
right?
It can be done, but I don't know whether it buys us much for the purely
theoretical case of new levels added.
Thanks,
tglx
On Sun, 2023-08-13 at 17:04 +0200, Thomas Gleixner wrote:
> On Sat, Aug 12 2023 at 22:04, Thomas Gleixner wrote:
> > On Sat, Aug 12 2023 at 08:21, Rui Zhang wrote:
> > > With this, we can guarantee that all the available topology
> > > information
> > > are always valid, even when running on future platforms.
> >
> > I know that it can be made work, but is it worth the extra effort?
> > I
> > don't think so.
>
> So I thought more about it. For intermediate levels, i.e. something
> which is squeezed between two existing levels, this works by some
> definition of works.
this "some definition of works" includes parsing the unknown levels,
right?
>
> I.e. the example where we have UBER_TILE between TILE and DIE, then
> we'd
> set and propagate the UBER_TILE entry into the DIE slot and then
> overwrite it again, if there is a DIE entry too.
Well, not really.
If we have TILE/UBER_TILE/DIE in CPUID but only support TILE/DIE in
kernel, the UBER_TILE information is overwritten.
But, UBER_TILE tells us the starting bit in APIC ID for die_id.
Say,
level type eax.shifts
0 SMT 1
1 CORE 5
2 TILE 7
3 UBER_TILE 8
4 DIE 9
This is a 1 package system with 2 dies, each die has 2 uber_tiles and
each uber_tile has 2 tiles.
If we don't support uber_tile, what we want to see is a platform with 2
dies and each die has 4 tiles.
But topo_shift_apicid() uses x86_topo_system.dom_shifts[TILE], so what
we see is a platform with 4 dies, and each die has 2 tiles. And this is
broken.
IMO, what we really need for each domain in x86_topo_system is dom_size
and dom_offset (id bit offset in APIC ID). and when parsing domain A,
we can propagate its eax.shifts to the dom_offset of its upper level
domains.
With this, we set dom_offset[DIE] to 7 first when parsing TILE, and
then overwrite it to 8 when parsing UBER_TILE, and set
dom_offset[PACKAGE] to 9 when parsinig DIE.
lossing TILE.eax.shifts is okay, because it is for UBER_TILE id.
>
> Where it becomes interesting is when the unknown level is past
> DIEGRP,
> e.g. DIEGRP_CONGLOMORATE then we'd need to overwrite the DIEGRP
> level,
> right?
>
> It can be done, but I don't know whether it buys us much for the
> purely
> theoretical case of new levels added.
>
>
Similar to previous case, DIEGRP_CONGLOMORATE eax.shifts can be
propagated to dom_offset[PACKAGE].
But, still, there is one case that we can not handle, (the reason I'm
proposing optional die support in Linux)
Say, we have new level FOO, and the CPUID is like this
level type eax.shifts
0 SMT 1
1 CORE 5
2 FOO 8
This can be a system with
1. 1 die and 8 FOOs if DIE is the upper level of FOO
or
2. 8 FOOs with 1 die in each FOO if DIE is the lower level of FOO
Currently, die topology information is mandatory in Linux, we cannot
make it right without patching enum topo_types/enum
x86_topology_domains/topo_domain_map (which in fact tells the
relationship between DIE and FOO).
thanks,
rui
> On Sun, 2023-08-13 at 17:04 +0200, Thomas Gleixner wrote:
>
> With this, we set dom_offset[DIE] to 7 first when parsing TILE, and
> then overwrite it to 8 when parsing UBER_TILE, and set
> dom_offset[PACKAGE] to 9 when parsinig DIE.
>
> lossing TILE.eax.shifts is okay, because it is for UBER_TILE id.
No. That's just wrong. TILE is defined and potentially used in the
kernel. How can you rightfully assume that UBER TILE is a valid
substitution? You can't.
> Currently, die topology information is mandatory in Linux, we cannot
> make it right without patching enum topo_types/enum
> x86_topology_domains/topo_domain_map (which in fact tells the
> relationship between DIE and FOO).
You cannot just nilly willy assume at which domain level FOO sits. Look
at your example:
> Say, we have new level FOO, and the CPUID is like this
> level type eax.shifts
> 0 SMT 1
> 1 CORE 5
> 2 FOO 8
FOO can be anything between CORE and PKG, so you cannot tell what it
means.
Simply heuristics _cannot_ be correct by definition. So why trying to
come up with them just because?
What's the problem you are trying to solve? Some real world issue or
some academic though experiment which might never become a real problem?
Thanks,
tglx
Len!
On Mon, Aug 14 2023 at 14:48, Len Brown wrote:
> First, with multiple cores having the same core_id.
> A consumer of core_id must know about packages to understand core_id.
>
> This is the original sin of the current interface -- which should
> never have used the word "sibling" *anyplace*, Because to make sense
> of the word sibling, you must know what *contains* the siblings.
You're conflating things. The fact that core_id is relative to the
package has absolutely nothing to do with the concept of siblings.
Whether sibling is the right terminology or not is a problem on its own.
Fact is that CPUs are structured in different domain levels and that
structuring makes some sense as it reflects the actual hardware.
The question whether this structuring has a relevance for software or
not, is independent of that. That's something which needs to be defined
and there are certainly aspects which affect scheduling, while others
affect power or other entities and some affect all of them.
> We introduced "core_cpus" a number of years ago to address this for
> core_ids (and for other levels, Such as die_cpus). Unfortunately, we
> can probably never actually delete threads_siblings and core_siblings
> Without breaking some program someplace...
Sorry, but "core_cpus" is just a different terminology. I'm not seeing
how this is solving anything.
> Core_id should be system-wide global, just like the cpu number is
> system wide global. Today, it is the only level id that is not system
> wide global.
That's simply not true. cpu_info::die_id is package relative too, which
is silly to begin with and caused to add this noisy logical_die_id muck.
> This could be implemented by simply not masking off the package_id
> bits when creating the core_id,
You have to shift it right by one, if the system is SMT capable, or just
use the APIC ID if it's not (i.e. 0x1f subleaf 0 has a shift of 0). Not
more not less.
Alternatively all IDs are not shifted right at all and just the bits
below the actual level are masked off.
> Like we have done for other levels.
Which one exactly? The only level ID which is truly system wide unique
is package ID.
Die ID is not and core ID is not and there are no other levels the
current upstream code is dealing with.
> Yes, this could be awkward for some existing code that indexes arrays
> with core_id, and doesn't like them to be sparse. But that rough edge
> is a much smaller problem than having to comprehend a level (package)
> that you may Otherwise not care about. Besides, core_id's can already
> be sparse today.
It's not awkward. It's just a matter of auditing all the places which
care about core ID and fixing them up in case they can't deal with it.
I went through the die ID usage before making die ID unique to ensure
that it wont break anything.
I surely have mopped up more complex things than that, so where is the
problem doing the same for core ID?
> Secondly, with the obsolescence of CPUID.0b and its replacement with
> CPUID.1F, the contract between The hardware and the software is that a
> level can appear and can in between any existing levels. (the only
> exception is that SMT is married to core).
In theory, yes. But what's the practical relevance that there might be a
new level between CORE and MODULE or MODULE and TILE etc...?
> It is not possible For an old kernel to know the name or position of a
> new level in the hierarchy, going forward.
Again, where is the practical problem? These new levels are not going to
be declared nilly willy and every other week, right?
> Today, this manifests with a (currently) latent bug that I caused.
> For I implemented die_id In the style of package_id, and I shouldn't
> have followed that example.
You did NOT. You implemented die_id relative to the package, which does
not make it unique in the same way as core_id is relative to the package
and therefore not unique.
Package ID is unique and the only reason why logical package ID exists
is because there have been systems with massive gaps between the package
IDs. That could have been handled differently, but that's again a
different story.
> Today, if CPUID.1F doesn't know anything about multiple DIE, Linux
> conjurs up A die_id 0 in sysfs. It should not. The reason is that
> when CPUID.1F enumerates A level that legacy code doesn't know about,
> we can't possibly tell if it is above DIE, or below DIE. If it is
> above DIE, then our default die_id 0 is becomes bogus.
That's an implementation problem and the code I posted fixes this by
making die_id unique and taking the documented domain levels into
account.
So if 0x1f does not enumerate dies, then each package has one die and
the die ID is the same as the package ID. It's that simple.
> That said, I have voiced my objection inside Intel to the creation of
> random levels Which do not have an architectural (software)
> definition; and I'm advocating that They be *removed* from the SDM
> until a software programming definition that Spans all generation is
> documented.
>
> SMT, core, module, die and the (implicit) package may not be well
> documented, But they do have existing uses and will thus live on. The
> others maybe not.
Why removing them? If there is no documentation for using them then
software will ignore them, but they reflect how the hardware is built
and according to conversations with various people this topology
reflects other things which are undocumented.
What do you win by removing them from the SDM?
Absolutely nothing. Actually you lose because if they get added with
information how software should use those levels then the whole problem
I discussed with Rui about imaginary new domain levels surfacing in the
future is there sooner than later. If we deal with them correctly today
and ignore them for then kernels will just work on systems which
enumerate them, they just wont make use of these levels.
The amount of extra storage to handle them is marginal and really not
worth to debate.
Thanks,
tglx
> What's the problem you are trying to solve? Some real world issue or some academic though experiment which might never become a real problem?
There are several existing bugs, bad practices, and latent future bugs in today's x86 topology code.
First, with multiple cores having the same core_id.
A consumer of core_id must know about packages to understand core_id.
This is the original sin of the current interface -- which should never have used the word "sibling" *anyplace*,
Because to make sense of the word sibling, you must know what *contains* the siblings.
We introduced "core_cpus" a number of years ago to address this for core_ids (and for other levels,
Such as die_cpus). Unfortunately, we can probably never actually delete threads_siblings and core_siblings
Without breaking some program someplace...
Core_id should be system-wide global, just like the cpu number is system wide global.
Today, it is the only level id that is not system wide global.
This could be implemented by simply not masking off the package_id bits when creating the core_id,
Like we have done for other levels.
Yes, this could be awkward for some existing code that indexes arrays with core_id, and doesn't like them to be sparse.
But that rough edge is a much smaller problem than having to comprehend a level (package) that you may
Otherwise not care about. Besides, core_id's can already be sparse today.
Secondly, with the obsolescence of CPUID.0b and its replacement with CPUID.1F, the contract between
The hardware and the software is that a level can appear and can in between any existing levels.
(the only exception is that SMT is married to core). It is not possible
For an old kernel to know the name or position of a new level in the hierarchy, going forward.
Today, this manifests with a (currently) latent bug that I caused. For I implemented die_id
In the style of package_id, and I shouldn't have followed that example.
Today, if CPUID.1F doesn't know anything about multiple DIE, Linux conjurs up
A die_id 0 in sysfs. It should not. The reason is that when CPUID.1F enumerates
A level that legacy code doesn't know about, we can't possibly tell if it is above DIE,
or below DIE. If it is above DIE, then our default die_id 0 is becomes bogus.
That said, I have voiced my objection inside Intel to the creation of random levels
Which do not have an architectural (software) definition; and I'm advocating that
They be *removed* from the SDM until a software programming definition that
Spans all generation is documented.
SMT, core, module, die and the (implicit) package may not be well documented,
But they do have existing uses and will thus live on.
The others maybe not.
-Len
On Mon, Aug 14 2023 at 15:28, Rui Zhang wrote:
> On Mon, 2023-08-14 at 14:26 +0200, Thomas Gleixner wrote:
>> What's the problem you are trying to solve? Some real world issue or
>> some academic though experiment which might never become a real
>> problem?
>>
> Maybe I was misleading previously, IMO, I totally agree with your
> points, and "using optional die/tile/module" is what I propose to
> address these concerns.
That's exactly what's implemented. If module, tile, die are not
advertised, then you end up with:
N threads
N/2 cores
1 module
1 tile
1 die
in a package because the bits occupied by module, tile and die are
exactly 0.
But from a conceptual and consistency point of view they exist, no?
Thanks,
tglx
Hi, Thomas,
On Mon, 2023-08-14 at 14:26 +0200, Thomas Gleixner wrote:
> > On Sun, 2023-08-13 at 17:04 +0200, Thomas Gleixner wrote:
> >
> > With this, we set dom_offset[DIE] to 7 first when parsing TILE, and
> > then overwrite it to 8 when parsing UBER_TILE, and set
> > dom_offset[PACKAGE] to 9 when parsinig DIE.
> >
> > lossing TILE.eax.shifts is okay, because it is for UBER_TILE id.
>
> No. That's just wrong. TILE is defined and potentially used in the
> kernel.
Sure.
> How can you rightfully assume that UBER TILE is a valid
> substitution? You can't.
TILE.eax.shifts tells
1. the number of maximum addressable threads in TILE domain, which
should be saved in x86_topo_system.dom_size[TILE]
2. the highest bit in APIC ID for tile id, but we don't need this if
we use package/system scope unique tile id
3. the lowest bit in APIC ID for the upper level of tile
if the upper level is a known level, say, die, this info is saved in
dom_offset[die]
if the upper level is an unknown level, then we don't need this to
decode the topology information for the unknown level.
maybe I missed something, for now I don't see how things break here.
>
> > Currently, die topology information is mandatory in Linux, we
> > cannot
> > make it right without patching enum topo_types/enum
> > x86_topology_domains/topo_domain_map (which in fact tells the
> > relationship between DIE and FOO).
>
> You cannot just nilly willy assume at which domain level FOO sits.
exactly.
> Look
> at your example:
>
> > Say, we have new level FOO, and the CPUID is like this
> > level type eax.shifts
> > 0 SMT 1
> > 1 CORE 5
> > 2 FOO 8
>
> FOO can be anything between CORE and PKG, so you cannot tell what it
> means.
Exactly. Anything related with MODULE/TILE/DIE can break in this case.
Say this is a system with 1 package, 2 FOOs, 8 cores.
In current design (in this patch set), kernel has to tell how many
dies/tiles/modules this system has, and kernel cannot do this right.
But if using optional Die (and surely optional module/tile), kernel can
tell that this is a 1-package-0-die-0-tile-0-module-8-core system
before knowing what FOO means, we don't need to make up anything we
don't know.
>
> Simply heuristics _cannot_ be correct by definition. So why trying to
> come up with them just because?
>
> What's the problem you are trying to solve? Some real world issue or
> some academic though experiment which might never become a real
> problem?
>
Maybe I was misleading previously, IMO, I totally agree with your
points, and "using optional die/tile/module" is what I propose to
address these concerns.
thanks,
rui
Len!
On Fri, Sep 01 2023 at 03:09, Len Brown wrote:
>> Conceptually _all_ levels exist, but the ones which occupy zero bits
>> have no meaning. Neither have the unknown levels if they should
>> surface at some point.
>>
>> So as they _all_ exist the logical consequence is that even those which occupy zero bits have an ID.
>>
>> Code which is interested in information which depends on the enumeration of the level must obviously do:
>>
>> if (level_exists(X))
>> analyse_level(X)
>>
>> Whether you express that via an invalid level ID or via an explicit
>> check for the level is an implementation detail.
>
> Thank you for acknowledging that a level with a shift-width of 0 does
> not exist, and thus an id for that level has no meaning.
Even if the level is enumerated then there is no implicit meaning
attached per se. It's only relevant when there is a documented
relationship between the enumeration and secondary information attached
to it. Making implicit general assumptions about the meaning of an
enumeration is just not possible,
> One could argue that except for package_id and core_id, which always
> exist, maintainable code would *always* check that a level exists
> before doing *anything* with its level_id. Color me skeptical of an
> implementation that does otherwise...
We have that today, no?
> So what are you proposing with the statement that "conceptually _all_
> levels exist"?
We need a consistent view on the topology and the only consistent view
is mathematical. Which means that a shift 0 element obviously has size
one because of size = 1 << SHIFT.
As a consequence these non-enumerated levels have an ID too, which in
turn makes the view on the topology consistent and independent of the
actually enumerated levels.
>> The problem of the current implementation is not that the die ID is
>> automatically assigned. The problem is at the usage sites which
>> blindly assume that there must be a meaning. That's a completely
>> different issue and has absolutely nothing to do with purely
>> mathematical deduced ID information at any given level.
>
> I agree that the code that exports the die_id attributes in topology
> sysfs should not do so when the die_id is meaningless.
The problem is not the fact that die_id is exposed. The problem is that
the meta information which allows to deduce meaning is not exposed along
with it. The fact that the exposure was half thought out makes is
slightly harder to correct that mistake, but I'm not yet convinced that
non-exposure is the correct answer in general.
> Ps. It is a safe bet that new levels will "surface at some point".
> For example, DieGrp surfaced this summer w/o any prior consultation
> with the Linux team. But even if they did consult us and gave us the
> ideal 1-year before-hardware advance notice, and even if we
> miraculously added support in 0 time, we would still be 2-years late
> to prescriptively recognize this new level -- as our enterprise
> customers routinely run 3-year-old kernels.
That's a strawman as the enterprise people backport the world and some
more. So if there is timely upstream support then it will turn up in the
frankenkernels in time too. Arguably we could even backport the new
magic level ID to stable kernels as well as we do with other important
hardware related minimal addons.
> This is why it is mandatory that our code be resilient to the
> insertion of additional future levels. I think it can be -- as long
> as we continue to use globally unique id's for all levels (IIR, only
> core_id is not globally unique today) and do _nothing_ with levels
> that have a 0 shift-width.
Die ID is relative too for no real good reason. Inside the kernel core
ID is not really required to be relative either.
Implementation wise it's just wrong to store this information in
cpu_info instead of doing a runtime evaluation of the topology
information, which allows to chose between global and relative IDs
depending on the requirements of the particular usage site.
The primary usage of these IDs is for initialization and everything
which needs this for hotpath usage converts it into a use case specific
cached representation anyway because accessing per cpu variables in a
hotpath is suboptimal at best.
Thanks,
tglx
Hello Thomas,
> Conceptually _all_ levels exist, but the ones which occupy zero bits have no meaning. Neither have the unknown levels if they should surface at some point.
>
> So as they _all_ exist the logical consequence is that even those which occupy zero bits have an ID.
>
> Code which is interested in information which depends on the enumeration of the level must obviously do:
>
> if (level_exists(X))
> analyse_level(X)
>
> Whether you express that via an invalid level ID or via an explicit check for the level is an implementation detail.
Thank you for acknowledging that a level with a shift-width of 0 does not exist, and thus an id for that level has no meaning.
One could argue that except for package_id and core_id, which always exist, maintainable code would *always* check that a level exists before doing *anything* with its level_id. Color me skeptical of an implementation that does otherwise...
So what are you proposing with the statement that "conceptually _all_ levels exist"?
> The problem of the current implementation is not that the die ID is automatically assigned. The problem is at the usage sites which blindly assume that there must be a meaning. That's a completely different issue and has absolutely nothing to do with purely mathematical deduced ID information at any given level.
I agree that the code that exports the die_id attributes in topology sysfs should not do so when the die_id is meaningless.
Thanks,
-Len
Ps. It is a safe bet that new levels will "surface at some point". For example, DieGrp surfaced this summer w/o any prior consultation with the Linux team. But even if they did consult us and gave us the ideal 1-year before-hardware advance notice, and even if we miraculously added support in 0 time, we would still be 2-years late to prescriptively recognize this new level -- as our enterprise customers routinely run 3-year-old kernels. This is why it is mandatory that our code be resilient to the insertion of additional future levels. I think it can be -- as long as we continue to use globally unique id's for all levels (IIR, only core_id is not globally unique today) and do _nothing_ with levels that have a 0 shift-width.