Re: [Patch v2 2/2] x86/tsc: use logical_packages as a better estimation of socket numbers
From: Zhang, Rui
Date: Fri Jun 23 2023 - 11:49:33 EST
On Fri, 2023-06-23 at 01:07 +0200, Thomas Gleixner wrote:
> On Thu, Jun 22 2023 at 16:27, Thomas Gleixner wrote:
> > On Fri, Jun 16 2023 at 15:18, Feng Tang wrote:
> > So something like the below should just work.
>
> Well it works in principle, but does not take any of the command line
> parameters which limit nr_possible CPUs or the actual kernel
> configuration into account. But the principle itself works correctly.
>
> Below is an updated version, which takes them into account.
>
> The data here is from a two socket system with 32 CPUs per socket.
>
> No command line parameters (NR_CPUS=64):
>
> smpboot: Allowing 64 CPUs, 32 hotplug CPUs
> clocksource: tsc-early: mask: 0xffffffffffffffff max_cycles:
> 0x1e3306b9ada, max_idle_ns: 440795224413 ns
> smp: Brought up 1 node, 32 CPUs
> smpboot: Max logical packages ACPI enumeration: 2
>
> "possible_cpus=32" (NR_CPUS=64) or
> No command line parameter (NR_CPUS=32):
>
> smpboot: Allowing 32 CPUs, 0 hotplug CPUs
> clocksource: tsc-early: mask: 0xffffffffffffffff max_cycles:
> 0x1e3306b9ada, max_idle_ns: 440795224413 ns
> smp: Brought up 1 node, 32 CPUs
> smpboot: Max logical packages ACPI enumeration: 1
>
> maxcpus=32
> smpboot: Allowing 64 CPUs, 0 hotplug CPUs
> clocksource: tsc-early: mask: 0xffffffffffffffff max_cycles:
> 0x1e3306b9ada, max_idle_ns: 440795224413 ns
> smp: Brought up 1 node, 32 CPUs
> smpboot: Max logical packages ACPI enumeration: 2
>
> But that's really all we should do. If the ACPI table enumerates CPUs
> as
> hotpluggable which can never arrive, then so be it.
>
> We have enough parameters to override the BIOS nonsense. Trying to do
> more magic MAD table parsing with heuristics is just wrong.
>
> We already have way too many heuristics and workarounds for broken
> firmware, but for the problem at hand, we really don't need more.
>
> The only systems I observed so far which have a non-sensical amount
> of
> "hotpluggable" CPUs are high-end server machines.
We see insane possible CPUs on IVB servers, one from Peter and one in
LKP lab, maybe a different problem but still related, because they may
cause bogus __max_logical_packages detected.
Below is fix I made but I don't have chance to test it yet.
From 77152bceb059944ee49ac0dc45e313354590c3ab Mon Sep 17 00:00:00 2001
From: Zhang Rui <rui.zhang@xxxxxxxxx>
Date: Fri, 23 Jun 2023 12:14:28 +0800
Subject: [PATCH RFC] x86/acpi: Ignore invalid x2APIC entries
According to https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#processor-local-x2apic-structure
"[Compatibility note] On some legacy OSes, Logical processors with APIC
ID values less than 255 (whether in XAPIC or X2APIC mode) must use the
Processor Local APIC structure to convey their APIC information to OSPM,
and those processors must be declared in the DSDT using the Processor()
keyword. Logical processors with APIC ID values 255 and greater must use
the Processor Local x2APIC structure and be declared using the Device()
keyword."
This means that if valid LAPIC entries are already detected, the APIC ID
in x2APIC must be equal to or larger than 0xff.
On an IVB-EP 2 sockets system with 20 CPUs per socket, Linux detects 40
present CPUs from LAPIC, and 80 possible CPUs from x2APIC, while many of
the x2APIC entries share the same APIC ID with LAPCI entries like below.
[02Ch 0044 1] Subtable Type : 00 [Processor Local APIC]
[02Fh 0047 1] Local Apic ID : 00
...
[164h 0356 1] Subtable Type : 00 [Processor Local APIC]
[167h 0359 1] Local Apic ID : 39
[16Ch 0364 1] Subtable Type : 00 [Processor Local APIC]
[16Fh 0367 1] Local Apic ID : FF
...
[3ECh 1004 1] Subtable Type : 09 [Processor Local x2APIC]
[3F0h 1008 4] Processor x2Apic ID : 00000000
...
[B5Ch 2908 1] Subtable Type : 09 [Processor Local x2APIC]
[B60h 2912 4] Processor x2Apic ID : 00000077
Follow the ACPI spec to ignore such x2APIC entries.
Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
---
arch/x86/kernel/acpi/boot.c | 35 ++++++++++++++++++-----------------
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 1c38174b5f01..9e06cc82ae95 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -199,6 +199,8 @@ static bool __init acpi_is_processor_usable(u32 lapic_flags)
return false;
}
+static bool has_lapic_cpus;
+
static int __init
acpi_parse_x2apic(union acpi_subtable_headers *header, const unsigned long end)
{
@@ -227,6 +229,14 @@ acpi_parse_x2apic(union acpi_subtable_headers *header, const unsigned long end)
if (!acpi_is_processor_usable(processor->lapic_flags))
return 0;
+ /*
+ * According to https://uefi.org/specs/ACPI/6.5/05_ACPI_Software_Programming_Model.html#processor-local-x2apic-structure
+ * when MADT provides both valid LAPIC and x2APIC entries, the APIC ID
+ * in x2APIC must be equal or greater than 0xff.
+ */
+ if (has_lapic_cpus && apic_id < 0xff)
+ return 0;
+
/*
* We need to register disabled CPU as well to permit
* counting disabled CPUs. This allows us to size
@@ -252,6 +262,7 @@ static int __init
acpi_parse_lapic(union acpi_subtable_headers * header, const unsigned long end)
{
struct acpi_madt_local_apic *processor = NULL;
+ int cpu;
processor = (struct acpi_madt_local_apic *)header;
@@ -275,10 +286,11 @@ acpi_parse_lapic(union acpi_subtable_headers * header, const unsigned long end)
* to not preallocating memory for all NR_CPUS
* when we use CPU hotplug.
*/
- acpi_register_lapic(processor->id, /* APIC ID */
+ cpu = acpi_register_lapic(processor->id, /* APIC ID */
processor->processor_id, /* ACPI ID */
processor->lapic_flags & ACPI_MADT_ENABLED);
-
+ if (cpu >= 0)
+ has_lapic_cpus = true;
return 0;
}
@@ -1118,21 +1130,10 @@ static int __init acpi_parse_madt_lapic_entries(void)
acpi_parse_sapic, MAX_LOCAL_APIC);
if (!count) {
- memset(madt_proc, 0, sizeof(madt_proc));
- madt_proc[0].id = ACPI_MADT_TYPE_LOCAL_APIC;
- madt_proc[0].handler = acpi_parse_lapic;
- madt_proc[1].id = ACPI_MADT_TYPE_LOCAL_X2APIC;
- madt_proc[1].handler = acpi_parse_x2apic;
- ret = acpi_table_parse_entries_array(ACPI_SIG_MADT,
- sizeof(struct acpi_table_madt),
- madt_proc, ARRAY_SIZE(madt_proc), MAX_LOCAL_APIC);
- if (ret < 0) {
- pr_err("Error parsing LAPIC/X2APIC entries\n");
- return ret;
- }
-
- count = madt_proc[0].count;
- x2count = madt_proc[1].count;
+ count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_LAPIC,
+ acpi_parse_lapic, MAX_LOCAL_APIC);
+ x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
+ acpi_parse_x2apic, MAX_LOCAL_APIC);
}
if (!count && !x2count) {
pr_err("No LAPIC entries present\n");
--
2.34.1