| Message ID | 1436261425-29881-4-git-send-email-tangchen@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
I think you forgot to reserve CPU 0 for BSP in cpuid mask. --Mika On Tue, Jul 7, 2015 at 12:30 PM, Tang Chen <tangchen@cn.fujitsu.com> wrote: > From: Gu Zheng <guz.fnst@cn.fujitsu.com> > > In this patch, we introduce a new static array named apicid_to_cpuid[], > 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 apicid_to_cpuid[], 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. > > > Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com> > Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com> > --- > arch/x86/include/asm/mpspec.h | 1 + > arch/x86/kernel/acpi/boot.c | 6 ++---- > arch/x86/kernel/apic/apic.c | 47 ++++++++++++++++++++++++++++++++++++++++--- > 3 files changed, 47 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h > index b07233b..db902d8 100644 > --- a/arch/x86/include/asm/mpspec.h > +++ b/arch/x86/include/asm/mpspec.h > @@ -86,6 +86,7 @@ static inline void early_reserve_e820_mpc_new(void) { } > #endif > > int generic_processor_info(int apicid, int version); > +int __generic_processor_info(int apicid, int version, bool enabled); > > #define PHYSID_ARRAY_SIZE BITS_TO_LONGS(MAX_LOCAL_APIC) > > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > index e49ee24..bcc85b2 100644 > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -174,15 +174,13 @@ static int acpi_register_lapic(int id, u8 enabled) > return -EINVAL; > } > > - if (!enabled) { > + if (!enabled) > ++disabled_cpus; > - return -EINVAL; > - } > > if (boot_cpu_physical_apicid != -1U) > ver = apic_version[boot_cpu_physical_apicid]; > > - return generic_processor_info(id, ver); > + return __generic_processor_info(id, ver, enabled); > } > > static int __init > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index a9c9830..c744ffb 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -1977,7 +1977,38 @@ void disconnect_bsp_APIC(int virt_wire_setup) > apic_write(APIC_LVT1, value); > } > > -static int __generic_processor_info(int apicid, int version, bool enabled) > +/* > + * Logic cpu number(cpuid) to local APIC id persistent mappings. > + * Do not clear the mapping even if cpu is hot-removed. > + */ > +static int apicid_to_cpuid[] = { > + [0 ... NR_CPUS - 1] = -1, > +}; > + > +/* > + * Internal cpu id bits, set the bit once cpu present, and never clear it. > + */ > +static cpumask_t cpuid_mask = CPU_MASK_NONE; > + > +static int get_cpuid(int apicid) > +{ > + int free_id, i; > + > + free_id = cpumask_next_zero(-1, &cpuid_mask); > + if (free_id >= nr_cpu_ids) > + return -1; > + > + for (i = 0; i < free_id; i++) > + if (apicid_to_cpuid[i] == apicid) > + return i; > + > + apicid_to_cpuid[free_id] = apicid; > + cpumask_set_cpu(free_id, &cpuid_mask); > + > + return free_id; > +} > + > +int __generic_processor_info(int apicid, int version, bool enabled) > { > int cpu, max = nr_cpu_ids; > bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid, > @@ -2058,8 +2089,18 @@ static int __generic_processor_info(int apicid, int version, bool enabled) > * for BSP. > */ > cpu = 0; > - } else > - cpu = cpumask_next_zero(-1, cpu_present_mask); > + } else { > + cpu = get_cpuid(apicid); > + if (cpu < 0) { > + int thiscpu = max + disabled_cpus; > + > + pr_warning(" Processor %d/0x%x ignored.\n", > + thiscpu, apicid); > + if (enabled) > + disabled_cpus++; > + return -EINVAL; > + } > + } > > /* > * Validate version > -- > 1.9.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mika, On 07/07/2015 07:14 PM, Mika Penttilä wrote: > I think you forgot to reserve CPU 0 for BSP in cpuid mask. Sorry for the late reply. I'm not familiar with BSP. Do you mean in get_cpuid(), I should reserve 0 for physical cpu0 in BSP ? Would you please share more detail ? Thanks. > > --Mika > > On Tue, Jul 7, 2015 at 12:30 PM, Tang Chen <tangchen@cn.fujitsu.com> wrote: >> From: Gu Zheng <guz.fnst@cn.fujitsu.com> >> >> In this patch, we introduce a new static array named apicid_to_cpuid[], >> 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 apicid_to_cpuid[], 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. >> >> >> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com> >> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com> >> --- >> arch/x86/include/asm/mpspec.h | 1 + >> arch/x86/kernel/acpi/boot.c | 6 ++---- >> arch/x86/kernel/apic/apic.c | 47 ++++++++++++++++++++++++++++++++++++++++--- >> 3 files changed, 47 insertions(+), 7 deletions(-) >> >> diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h >> index b07233b..db902d8 100644 >> --- a/arch/x86/include/asm/mpspec.h >> +++ b/arch/x86/include/asm/mpspec.h >> @@ -86,6 +86,7 @@ static inline void early_reserve_e820_mpc_new(void) { } >> #endif >> >> int generic_processor_info(int apicid, int version); >> +int __generic_processor_info(int apicid, int version, bool enabled); >> >> #define PHYSID_ARRAY_SIZE BITS_TO_LONGS(MAX_LOCAL_APIC) >> >> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c >> index e49ee24..bcc85b2 100644 >> --- a/arch/x86/kernel/acpi/boot.c >> +++ b/arch/x86/kernel/acpi/boot.c >> @@ -174,15 +174,13 @@ static int acpi_register_lapic(int id, u8 enabled) >> return -EINVAL; >> } >> >> - if (!enabled) { >> + if (!enabled) >> ++disabled_cpus; >> - return -EINVAL; >> - } >> >> if (boot_cpu_physical_apicid != -1U) >> ver = apic_version[boot_cpu_physical_apicid]; >> >> - return generic_processor_info(id, ver); >> + return __generic_processor_info(id, ver, enabled); >> } >> >> static int __init >> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c >> index a9c9830..c744ffb 100644 >> --- a/arch/x86/kernel/apic/apic.c >> +++ b/arch/x86/kernel/apic/apic.c >> @@ -1977,7 +1977,38 @@ void disconnect_bsp_APIC(int virt_wire_setup) >> apic_write(APIC_LVT1, value); >> } >> >> -static int __generic_processor_info(int apicid, int version, bool enabled) >> +/* >> + * Logic cpu number(cpuid) to local APIC id persistent mappings. >> + * Do not clear the mapping even if cpu is hot-removed. >> + */ >> +static int apicid_to_cpuid[] = { >> + [0 ... NR_CPUS - 1] = -1, >> +}; >> + >> +/* >> + * Internal cpu id bits, set the bit once cpu present, and never clear it. >> + */ >> +static cpumask_t cpuid_mask = CPU_MASK_NONE; >> + >> +static int get_cpuid(int apicid) >> +{ >> + int free_id, i; >> + >> + free_id = cpumask_next_zero(-1, &cpuid_mask); >> + if (free_id >= nr_cpu_ids) >> + return -1; >> + >> + for (i = 0; i < free_id; i++) >> + if (apicid_to_cpuid[i] == apicid) >> + return i; >> + >> + apicid_to_cpuid[free_id] = apicid; >> + cpumask_set_cpu(free_id, &cpuid_mask); >> + >> + return free_id; >> +} >> + >> +int __generic_processor_info(int apicid, int version, bool enabled) >> { >> int cpu, max = nr_cpu_ids; >> bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid, >> @@ -2058,8 +2089,18 @@ static int __generic_processor_info(int apicid, int version, bool enabled) >> * for BSP. >> */ >> cpu = 0; >> - } else >> - cpu = cpumask_next_zero(-1, cpu_present_mask); >> + } else { >> + cpu = get_cpuid(apicid); >> + if (cpu < 0) { >> + int thiscpu = max + disabled_cpus; >> + >> + pr_warning(" Processor %d/0x%x ignored.\n", >> + thiscpu, apicid); >> + if (enabled) >> + disabled_cpus++; >> + return -EINVAL; >> + } >> + } >> >> /* >> * Validate version >> -- >> 1.9.3 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ > . > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/7/15 11:33, Tang Chen wrote: > Hi Mika, > > On 07/07/2015 07:14 PM, Mika Penttilä wrote: >> I think you forgot to reserve CPU 0 for BSP in cpuid mask. > > Sorry for the late reply. > > I'm not familiar with BSP. Do you mean in get_cpuid(), > I should reserve 0 for physical cpu0 in BSP ? > > Would you please share more detail ? BSP stands for "Bootstrapping Processor". In other word, BSP is CPU0. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/15/2015 01:35 PM, Jiang Liu wrote: > On 2015/7/15 11:33, Tang Chen wrote: >> Hi Mika, >> >> On 07/07/2015 07:14 PM, Mika Penttilä wrote: >>> I think you forgot to reserve CPU 0 for BSP in cpuid mask. >> Sorry for the late reply. >> >> I'm not familiar with BSP. Do you mean in get_cpuid(), >> I should reserve 0 for physical cpu0 in BSP ? >> >> Would you please share more detail ? > BSP stands for "Bootstrapping Processor". In other word, > BSP is CPU0. > . > Ha, how foolish I am. And yes, cpu0 is not reserved when apicid == boot_cpu_physical_apicid comes true. Will update the patch. Thanks. :) -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Tue, Jul 07, 2015 at 05:30:23PM +0800, Tang Chen wrote: > From: Gu Zheng <guz.fnst@cn.fujitsu.com> It would be a good idea to briefly describe what the overall goal is and why we want that. > In this patch, we introduce a new static array named apicid_to_cpuid[], > 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 apicid_to_cpuid[], 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. > > > Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com> > Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com> > --- ... > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c > index e49ee24..bcc85b2 100644 > --- a/arch/x86/kernel/acpi/boot.c > +++ b/arch/x86/kernel/acpi/boot.c > @@ -174,15 +174,13 @@ static int acpi_register_lapic(int id, u8 enabled) > return -EINVAL; > } > > - if (!enabled) { > + if (!enabled) > ++disabled_cpus; > - return -EINVAL; > - } > > if (boot_cpu_physical_apicid != -1U) > ver = apic_version[boot_cpu_physical_apicid]; > > - return generic_processor_info(id, ver); > + return __generic_processor_info(id, ver, enabled); > } > > static int __init > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index a9c9830..c744ffb 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c > @@ -1977,7 +1977,38 @@ void disconnect_bsp_APIC(int virt_wire_setup) > apic_write(APIC_LVT1, value); > } > > -static int __generic_processor_info(int apicid, int version, bool enabled) > +/* > + * Logic cpu number(cpuid) to local APIC id persistent mappings. Logical Also, isn't it the other way around? > + * Do not clear the mapping even if cpu is hot-removed. > + */ > +static int apicid_to_cpuid[] = { > + [0 ... NR_CPUS - 1] = -1, > +}; > + > +/* > + * Internal cpu id bits, set the bit once cpu present, and never clear it. > + */ > +static cpumask_t cpuid_mask = CPU_MASK_NONE; > + > +static int get_cpuid(int apicid) > +{ > + int free_id, i; > + > + free_id = cpumask_next_zero(-1, &cpuid_mask); > + if (free_id >= nr_cpu_ids) > + return -1; > + > + for (i = 0; i < free_id; i++) > + if (apicid_to_cpuid[i] == apicid) > + return i; > + > + apicid_to_cpuid[free_id] = apicid; > + cpumask_set_cpu(free_id, &cpuid_mask); > + > + return free_id; Why can't this function simply test whether apicid_to_cpuid[] is -1 or not? Also, why does it need cpuid_mask? Isn't it just giving out cpu id numbers sequentially? > +} > + > +int __generic_processor_info(int apicid, int version, bool enabled) > { > int cpu, max = nr_cpu_ids; > bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid, > @@ -2058,8 +2089,18 @@ static int __generic_processor_info(int apicid, int version, bool enabled) > * for BSP. > */ > cpu = 0; > - } else > - cpu = cpumask_next_zero(-1, cpu_present_mask); > + } else { > + cpu = get_cpuid(apicid); > + if (cpu < 0) { > + int thiscpu = max + disabled_cpus; > + > + pr_warning(" Processor %d/0x%x ignored.\n", > + thiscpu, apicid); Given that the only failing condition is there are more possible cpus than nr_cpu_ids, it might make more sense to warn this once in get_cpuid(). Also, wouldn't it make more sense / safer to allocate all online cpus first and then go through possible cpus? Thanks.
diff --git a/arch/x86/include/asm/mpspec.h b/arch/x86/include/asm/mpspec.h index b07233b..db902d8 100644 --- a/arch/x86/include/asm/mpspec.h +++ b/arch/x86/include/asm/mpspec.h @@ -86,6 +86,7 @@ static inline void early_reserve_e820_mpc_new(void) { } #endif int generic_processor_info(int apicid, int version); +int __generic_processor_info(int apicid, int version, bool enabled); #define PHYSID_ARRAY_SIZE BITS_TO_LONGS(MAX_LOCAL_APIC) diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index e49ee24..bcc85b2 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -174,15 +174,13 @@ static int acpi_register_lapic(int id, u8 enabled) return -EINVAL; } - if (!enabled) { + if (!enabled) ++disabled_cpus; - return -EINVAL; - } if (boot_cpu_physical_apicid != -1U) ver = apic_version[boot_cpu_physical_apicid]; - return generic_processor_info(id, ver); + return __generic_processor_info(id, ver, enabled); } static int __init diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c index a9c9830..c744ffb 100644 --- a/arch/x86/kernel/apic/apic.c +++ b/arch/x86/kernel/apic/apic.c @@ -1977,7 +1977,38 @@ void disconnect_bsp_APIC(int virt_wire_setup) apic_write(APIC_LVT1, value); } -static int __generic_processor_info(int apicid, int version, bool enabled) +/* + * Logic cpu number(cpuid) to local APIC id persistent mappings. + * Do not clear the mapping even if cpu is hot-removed. + */ +static int apicid_to_cpuid[] = { + [0 ... NR_CPUS - 1] = -1, +}; + +/* + * Internal cpu id bits, set the bit once cpu present, and never clear it. + */ +static cpumask_t cpuid_mask = CPU_MASK_NONE; + +static int get_cpuid(int apicid) +{ + int free_id, i; + + free_id = cpumask_next_zero(-1, &cpuid_mask); + if (free_id >= nr_cpu_ids) + return -1; + + for (i = 0; i < free_id; i++) + if (apicid_to_cpuid[i] == apicid) + return i; + + apicid_to_cpuid[free_id] = apicid; + cpumask_set_cpu(free_id, &cpuid_mask); + + return free_id; +} + +int __generic_processor_info(int apicid, int version, bool enabled) { int cpu, max = nr_cpu_ids; bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid, @@ -2058,8 +2089,18 @@ static int __generic_processor_info(int apicid, int version, bool enabled) * for BSP. */ cpu = 0; - } else - cpu = cpumask_next_zero(-1, cpu_present_mask); + } else { + cpu = get_cpuid(apicid); + if (cpu < 0) { + int thiscpu = max + disabled_cpus; + + pr_warning(" Processor %d/0x%x ignored.\n", + thiscpu, apicid); + if (enabled) + disabled_cpus++; + return -EINVAL; + } + } /* * Validate version