On Thu, Sep 22, 2016 at 12:10 PM, tip-bot for Gu Zheng <[email protected]> wrote:
>
> x86/acpi: Introduce persistent storage for cpuid <-> apicid mapping
>
> The whole patch-set aims at making cpuid <-> nodeid mapping persistent. So that,
> when node online/offline happens, cache based on cpuid <-> nodeid mapping such as
> wq_numa_possible_cpumask will not cause any problem.
> It contains 4 steps:
> 1. Enable apic registeration flow to handle both enabled and disabled cpus.
> 2. Introduce a new array storing all possible cpuid <-> apicid mapping.
> 3. Enable _MAT and MADT relative apis to return non-present or disabled cpus' apicid.
> 4. Establish all possible cpuid <-> nodeid mapping.
>
> This patch finishes step 2.
>
> In this patch, we introduce a new static array named cpuid_to_apicid[],
> which is large enough to store info for all possible cpus.
>
> And then, we modify the cpuid calculation. In generic_processor_info(),
> it simply finds the next unused cpuid. And it is also why the cpuid <-> nodeid
> mapping changes with node hotplug.
>
> After this patch, we find the next unused cpuid, map it to an apicid,
> and store the mapping in cpuid_to_apicid[], so that cpuid <-> apicid
> mapping will be persistent.
>
> And finally we will use this array to make cpuid <-> nodeid persistent.
>
> cpuid <-> apicid mapping is established at local apic registeration time.
> But non-present or disabled cpus are ignored.
>
> In this patch, we establish all possible cpuid <-> apicid mapping when
> registering local apic.
Hi,
This one cause one regression on 8 sockets system: MLC from intel does not run
anymore.
the root cause is : cpu index used to be 0-447.
with this patch, cpu index change to 0, 2-448.
The MADT from system is like:
[ 42.107902] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[ 42.120125] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[ 42.132361] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[ 42.144598] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[ 42.156836] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
...
[ 47.552852] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[ 47.565088] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[ 47.577322] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[ 47.589561] ACPI: X2APIC (uid[0x00] apic_id[0x00] enabled)
[ 47.600899] ACPI: X2APIC (uid[0x02] apic_id[0x02] enabled)
[ 47.612234] ACPI: X2APIC (uid[0x04] apic_id[0x04] enabled)
...
init_cpu_node become:
[ 55.477160] init_cpu_to_node:
[ 55.483280] cpu 0 -> apicid 0x0 -> node 0
[ 55.491558] cpu 1 -> apicid 0xff -> node 1
[ 55.500017] cpu 2 -> apicid 0x2 -> node 0
[ 55.508296] cpu 3 -> apicid 0x4 -> node 0
[ 55.516575] cpu 4 -> apicid 0x6 -> node 0
...
looks like problem is
acpi_parse_lapic==>acpi_register_lapic==>__generic_processor_info==>allocate_logical_cpuid
it will take lapic_id[0xff] take cpu index 1.
Then will have not /dev/cpu/1/msr, that will make the MLC not happy.
Following change could workaround the problem at this point.
Index: linux-2.6/arch/x86/kernel/acpi/boot.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/acpi/boot.c
+++ linux-2.6/arch/x86/kernel/acpi/boot.c
@@ -163,10 +163,11 @@ static int __init acpi_parse_madt(struct
* @id: local apic id to register
* @acpiid: ACPI id to register
* @enabled: this cpu is enabled or not
+ * @disabled_id: not used apic id
*
* Returns the logic cpu number which maps to the local apic
*/
-static int acpi_register_lapic(int id, u32 acpiid, u8 enabled)
+static int acpi_register_lapic(int id, u32 acpiid, u8 enabled, int disabled_id)
{
unsigned int ver = 0;
int cpu;
@@ -176,6 +177,11 @@ static int acpi_register_lapic(int id, u
return -EINVAL;
}
+ if (!enabled && (id == disabled_id)) {
+ ++disabled_cpus;
+ return -EINVAL;
+ }
+
if (boot_cpu_physical_apicid != -1U)
ver = boot_cpu_apic_version;
@@ -213,7 +219,7 @@ acpi_parse_x2apic(struct acpi_subtable_h
if (!apic->apic_id_valid(apic_id) && enabled)
printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
else
- acpi_register_lapic(apic_id, processor->uid, enabled);
+ acpi_register_lapic(apic_id, processor->uid, enabled, -1);
#else
printk(KERN_WARNING PREFIX "x2apic entry ignored\n");
#endif
@@ -242,7 +248,7 @@ acpi_parse_lapic(struct acpi_subtable_he
*/
acpi_register_lapic(processor->id, /* APIC ID */
processor->processor_id, /* ACPI ID */
- processor->lapic_flags & ACPI_MADT_ENABLED);
+ processor->lapic_flags & ACPI_MADT_ENABLED, 0xff);
return 0;
}
@@ -261,7 +267,7 @@ acpi_parse_sapic(struct acpi_subtable_he
acpi_register_lapic((processor->id << 8) | processor->eid,/* APIC ID */
processor->processor_id, /* ACPI ID */
- processor->lapic_flags & ACPI_MADT_ENABLED);
+ processor->lapic_flags & ACPI_MADT_ENABLED, -1);
return 0;
}
@@ -725,7 +731,7 @@ int acpi_map_cpu(acpi_handle handle, phy
{
int cpu;
- cpu = acpi_register_lapic(physid, U32_MAX, ACPI_MADT_ENABLED);
+ cpu = acpi_register_lapic(physid, U32_MAX, ACPI_MADT_ENABLED, -1);
if (cpu < 0) {
pr_info(PREFIX "Unable to map lapic to logical cpu number\n");
return cpu;
On Mon, 3 Oct 2016, Yinghai Lu wrote:
> init_cpu_node become:
> [ 55.477160] init_cpu_to_node:
> [ 55.483280] cpu 0 -> apicid 0x0 -> node 0
> [ 55.491558] cpu 1 -> apicid 0xff -> node 1
> [ 55.500017] cpu 2 -> apicid 0x2 -> node 0
> [ 55.508296] cpu 3 -> apicid 0x4 -> node 0
> [ 55.516575] cpu 4 -> apicid 0x6 -> node 0
> ...
>
> looks like problem is
>
> acpi_parse_lapic==>acpi_register_lapic==>__generic_processor_info==>allocate_logical_cpuid
>
> it will take lapic_id[0xff] take cpu index 1.
>
> Then will have not /dev/cpu/1/msr, that will make the MLC not happy.
That's pretty irrelevant whether that MLC thing - whatever it is - is
unhappy or not. If it cannot deal with something missing, it's surely not
the kernels problem. Unplug the cpu and the file will be missing as well.
> -static int acpi_register_lapic(int id, u32 acpiid, u8 enabled)
> +static int acpi_register_lapic(int id, u32 acpiid, u8 enabled, int disabled_id)
> {
> unsigned int ver = 0;
> int cpu;
This is whitespace damaged. The patch does not apply. Can you finaly after
doing 10 years of kernel development start being more careful?
> @@ -176,6 +177,11 @@ static int acpi_register_lapic(int id, u
> return -EINVAL;
> }
>
> + if (!enabled && (id == disabled_id)) {
> + ++disabled_cpus;
> + return -EINVAL;
> + }
Why would you need that disabled_id thing at all? The proper fix is to let
the apic driver detect the issue and this boils down to a 5 lines
change. Does the patch below fix the issue for you?
Thanks,
tglx
8<----------------
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2076,6 +2076,11 @@ int __generic_processor_info(int apicid,
bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
phys_cpu_present_map);
+ if (!apic->apic_id_valid(apicid)) {
+ disabled_cpus++;
+ return -EINVAL;
+ }
+
/*
* boot_cpu_physical_apicid is designed to have the apicid
* returned by read_apic_id(), i.e, the apicid of the
On Wed, Oct 5, 2016 at 7:04 AM, Thomas Gleixner <[email protected]> wrote:
>> @@ -176,6 +177,11 @@ static int acpi_register_lapic(int id, u
>> return -EINVAL;
>> }
>>
>> + if (!enabled && (id == disabled_id)) {
>> + ++disabled_cpus;
>> + return -EINVAL;
>> + }
>
> Why would you need that disabled_id thing at all? The proper fix is to let
> the apic driver detect the issue and this boils down to a 5 lines
> change. Does the patch below fix the issue for you?
> 8<----------------
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -2076,6 +2076,11 @@ int __generic_processor_info(int apicid,
> bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
> phys_cpu_present_map);
>
> + if (!apic->apic_id_valid(apicid)) {
> + disabled_cpus++;
> + return -EINVAL;
> + }
> +
> /*
> * boot_cpu_physical_apicid is designed to have the apicid
> * returned by read_apic_id(), i.e, the apicid of the
>
>
No, That does not fix the issue.
the system have x2apic pre_enabled from BIOS, so at the time
apic is set to &apic_x2apic_cluster.
early_acpi_boot_init ==> early_acpi_process_madt ==> acpi_parse_madt
==> default_acpi_madt_oem_check
default_acpi_madt_oem_check
==> apic_x2apic_cluster/x2apic_acpi_madt_oem_check ==> x2apic_enabled
==> apic = &apic_x2apic_cluster
and
static int x2apic_apic_id_valid(int apicid)
{
return 1;
}
To make your change work, may need to update x2apic_apic_id_valid to
static int x2apic_apic_id_valid(int apicid)
{
if (apicid == 0xff || apicid == -1)
return 0;
return 1;
}
Thanks
Yinghai
Hi Yinghai,
At 10/06/2016 12:53 PM, Yinghai Lu wrote:
> On Wed, Oct 5, 2016 at 7:04 AM, Thomas Gleixner <[email protected]> wrote:
>>> @@ -176,6 +177,11 @@ static int acpi_register_lapic(int id, u
>>> return -EINVAL;
>>> }
>>>
>>> + if (!enabled && (id == disabled_id)) {
>>> + ++disabled_cpus;
>>> + return -EINVAL;
>>> + }
>>
>> Why would you need that disabled_id thing at all? The proper fix is to let
>> the apic driver detect the issue and this boils down to a 5 lines
>> change. Does the patch below fix the issue for you?
>> 8<----------------
>> --- a/arch/x86/kernel/apic/apic.c
>> +++ b/arch/x86/kernel/apic/apic.c
>> @@ -2076,6 +2076,11 @@ int __generic_processor_info(int apicid,
>> bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
>> phys_cpu_present_map);
>>
>> + if (!apic->apic_id_valid(apicid)) {
>> + disabled_cpus++;
>> + return -EINVAL;
>> + }
>> +
>> /*
>> * boot_cpu_physical_apicid is designed to have the apicid
>> * returned by read_apic_id(), i.e, the apicid of the
>>
>>
>
> No, That does not fix the issue.
>
> the system have x2apic pre_enabled from BIOS, so at the time
> apic is set to &apic_x2apic_cluster.
>
> early_acpi_boot_init ==> early_acpi_process_madt ==> acpi_parse_madt
> ==> default_acpi_madt_oem_check
>
> default_acpi_madt_oem_check
> ==> apic_x2apic_cluster/x2apic_acpi_madt_oem_check ==> x2apic_enabled
> ==> apic = &apic_x2apic_cluster
>
> and
> static int x2apic_apic_id_valid(int apicid)
> {
> return 1;
> }
>
> To make your change work, may need to update x2apic_apic_id_valid to
>
> static int x2apic_apic_id_valid(int apicid)
> {
> if (apicid == 0xff || apicid == -1)
> return 0;
>
I seem to remember that in x2APIC Spec the x2APIC ID may be at 255 or
greater.
If we do that judgment, it may be affect x2APIC's work in some other places.
I saw the MADT, the main reason may be that we define 0xff to acpi_id
in LAPIC mode.
As you said, it was like:
[ 42.107902] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[ 42.120125] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
[ 42.132361] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
...
How about doing the acpi_id check when we parse it in
acpi_parse_lapic().
8<----------------
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -233,6 +233,11 @@ acpi_parse_lapic(struct acpi_subtable_header *
header, const unsigned long end)
acpi_table_print_madt_entry(header);
+ if (processor->id >= 255) {
+ ++disabled_cpus;
+ return -EINVAL;
+ }
+
/*
* We need to register disabled CPU as well to permit
* counting disabled CPUs. This allows us to size
Thanks
Dou
> return 1;
> }
>
>
> Thanks
>
> Yinghai
>
>
On Thu, Oct 6, 2016 at 1:06 AM, Dou Liyang <[email protected]> wrote:
> I seem to remember that in x2APIC Spec the x2APIC ID may be at 255 or
> greater.
Good to know. Maybe later when one package have more cores like 30 cores etc.
> If we do that judgment, it may be affect x2APIC's work in some other places.
>
> I saw the MADT, the main reason may be that we define 0xff to acpi_id
> in LAPIC mode.
> As you said, it was like:
> [ 42.107902] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
> [ 42.120125] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
> [ 42.132361] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
> ...
>
> How about doing the acpi_id check when we parse it in
> acpi_parse_lapic().
>
> 8<----------------
>
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -233,6 +233,11 @@ acpi_parse_lapic(struct acpi_subtable_header * header,
> const unsigned long end)
>
> acpi_table_print_madt_entry(header);
>
> + if (processor->id >= 255) {
> + ++disabled_cpus;
> + return -EINVAL;
> + }
> +
> /*
> * We need to register disabled CPU as well to permit
> * counting disabled CPUs. This allows us to size
Yes, that should work. but should do the same thing for x2apic
in acpi_parse_x2apic should have
> + if (processor->local_apic_id == -1) {
> + ++disabled_cpus;
> + return -EINVAL;
> + }
that is the reason why i want to extend acpi_register_lapic()
to take extra disabled_id (one is 0xff and another is 0xffffffff)
so could save some lines.
Thanks
Yinghai
Hi Yinghai
At 10/07/2016 05:20 AM, Yinghai Lu wrote:
> On Thu, Oct 6, 2016 at 1:06 AM, Dou Liyang <[email protected]> wrote:
>
>> I seem to remember that in x2APIC Spec the x2APIC ID may be at 255 or
>> greater.
>
> Good to know. Maybe later when one package have more cores like 30 cores etc.
>
>> If we do that judgment, it may be affect x2APIC's work in some other places.
>>
>> I saw the MADT, the main reason may be that we define 0xff to acpi_id
>> in LAPIC mode.
>> As you said, it was like:
>> [ 42.107902] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
>> [ 42.120125] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
>> [ 42.132361] ACPI: LAPIC (acpi_id[0xff] lapic_id[0xff] disabled)
>> ...
>>
>> How about doing the acpi_id check when we parse it in
>> acpi_parse_lapic().
>>
>> 8<----------------
>>
>> --- a/arch/x86/kernel/acpi/boot.c
>> +++ b/arch/x86/kernel/acpi/boot.c
>> @@ -233,6 +233,11 @@ acpi_parse_lapic(struct acpi_subtable_header * header,
>> const unsigned long end)
>>
>> acpi_table_print_madt_entry(header);
>>
>> + if (processor->id >= 255) {
>> + ++disabled_cpus;
>> + return -EINVAL;
>> + }
>> +
>> /*
>> * We need to register disabled CPU as well to permit
>> * counting disabled CPUs. This allows us to size
>
> Yes, that should work. but should do the same thing for x2apic
>
> in acpi_parse_x2apic should have
>
>> + if (processor->local_apic_id == -1) {
>> + ++disabled_cpus;
>> + return -EINVAL;
>> + }
>
> that is the reason why i want to extend acpi_register_lapic()
> to take extra disabled_id (one is 0xff and another is 0xffffffff)
> so could save some lines.
>
Yes, I understood.
But I think adding an extra disabled_id is not a good way for
validating the apic_id. If the disabled_id is not just one id(-1 or
255), may be two or more, even be a range. what should we do for
extending our code?
Firstly, I am not sure that the "-1" could appear in the MADT, even if
the ACPI tables is unreasonable.
Seondly, I guess if we need the check, there are some reserved methods
in the kernel, such as "default_apic_id_valid", "x2apic_apic_id_valid"
and so on. we should extend all of them and use them for check.
CC'ed: Rafael and Lv
May I ask a question?
Is it possible that the "-1/oxffffffff" could appear in the MADT which
is one of the ACPI tables?
> Thanks
>
> Yinghai
>
>
On Thu, 6 Oct 2016, Yinghai Lu wrote:
> On Thu, Oct 6, 2016 at 1:06 AM, Dou Liyang <[email protected]> wrote:
> Yes, that should work. but should do the same thing for x2apic
>
> in acpi_parse_x2apic should have
>
> > + if (processor->local_apic_id == -1) {
> > + ++disabled_cpus;
> > + return -EINVAL;
> > + }
>
> that is the reason why i want to extend acpi_register_lapic()
> to take extra disabled_id (one is 0xff and another is 0xffffffff)
> so could save some lines.
That just makes the code harder to follow and is not really worth the
trouble.
Thanks,
tglx
On Thu, 6 Oct 2016, Yinghai Lu wrote:
> Yes, that should work. but should do the same thing for x2apic
>
> in acpi_parse_x2apic should have
>
> > + if (processor->local_apic_id == -1) {
> > + ++disabled_cpus;
> > + return -EINVAL;
> > + }
So that means, that x2apic_apic_id_valid() is wrong as well ....
On Fri, 7 Oct 2016, Dou Liyang wrote:
> Is it possible that the "-1/oxffffffff" could appear in the MADT which is one
> of the ACPI tables?
According to the SDM the x2apic id is a 32bit ID, so 0xffffffff is a
legitimate value.
Thanks,
tglx
On Fri, 7 Oct 2016, Thomas Gleixner wrote:
> On Fri, 7 Oct 2016, Dou Liyang wrote:
> > Is it possible that the "-1/oxffffffff" could appear in the MADT which is one
> > of the ACPI tables?
>
> According to the SDM the x2apic id is a 32bit ID, so 0xffffffff is a
> legitimate value.
The ACPI spec says that bit 0 of the x2apic flags field tells whether the
logical processor is present or not. So the proper check for x2apic is that
flag.
The lapic structure has the same flag, but the kernel ignores the flags for
both lapic and x2apic.
I'm going to apply the minimal fix of checking for id == 0xff in
acpi_lapic_parse() for now, but this needs to be revisited and fixed
proper.
Thanks,
tglx
On Thu, 6 Oct 2016, Dou Liyang wrote:
>
> + if (processor->id >= 255) {
> + ++disabled_cpus;
Incrementing disabled_cpus here is simply wrong because 0xff is an invalid
local APIC id. So we can simply return -EINVAL and be done with it.
> + return -EINVAL;
Thanks,
tglx
On Fri, Oct 7, 2016 at 6:00 AM, Thomas Gleixner <[email protected]> wrote:
> On Fri, 7 Oct 2016, Thomas Gleixner wrote:
>> On Fri, 7 Oct 2016, Dou Liyang wrote:
>> > Is it possible that the "-1/oxffffffff" could appear in the MADT which is one
>> > of the ACPI tables?
>>
>> According to the SDM the x2apic id is a 32bit ID, so 0xffffffff is a
>> legitimate value.
>
> The ACPI spec says that bit 0 of the x2apic flags field tells whether the
> logical processor is present or not. So the proper check for x2apic is that
> flag.
>
> The lapic structure has the same flag, but the kernel ignores the flags for
> both lapic and x2apic.
>
> I'm going to apply the minimal fix of checking for id == 0xff in
> acpi_lapic_parse() for now, but this needs to be revisited and fixed
> proper.
Good to me. Thanks for fixing it.
Yinghai
Hi tglx,
At 10/07/2016 09:07 PM, Thomas Gleixner wrote:
> On Thu, 6 Oct 2016, Dou Liyang wrote:
>>
>> + if (processor->id >= 255) {
>> + ++disabled_cpus;
>
> Incrementing disabled_cpus here is simply wrong because 0xff is an invalid
> local APIC id. So we can simply return -EINVAL and be done with it.
>
Yes, It is.
>> + return -EINVAL;
>
> Thanks,
>
> tglx
>
>
Thanks,
Dou
Hi tglx,
At 10/07/2016 09:00 PM, Thomas Gleixner wrote:
> On Fri, 7 Oct 2016, Thomas Gleixner wrote:
>> On Fri, 7 Oct 2016, Dou Liyang wrote:
>>> Is it possible that the "-1/oxffffffff" could appear in the MADT which is one
>>> of the ACPI tables?
>>
>> According to the SDM the x2apic id is a 32bit ID, so 0xffffffff is a
>> legitimate value.
Yes, I see.
>
> The ACPI spec says that bit 0 of the x2apic flags field tells whether the
> logical processor is present or not. So the proper check for x2apic is that
> flag.
>
> The lapic structure has the same flag, but the kernel ignores the flags for
> both lapic and x2apic.
It seems the kernel uses the flags in this sentence:
enabled = processor->lapic_flags & ACPI_MADT_ENABLED;
>
> I'm going to apply the minimal fix of checking for id == 0xff in
> acpi_lapic_parse() for now, but this needs to be revisited and fixed
> proper.
Yes, I will do it.
Thanks
Dou.