Recently, we hoped to make the possible CPU count more accurate for
Kernel. I stuck on the issue how do I run acpi_early_init() _before_
setup_percpu() is invoked. So send these insignificant patches first.
This patchset does this things:
- two document-related work(the 1th and 2th patch),
- two cleapup work (the 3th and 5th patch)
- a bug fix for CPU hotplug(4th patch)
Dou Liyang (5):
x86/smpboot: Add the missing description of possible_cpus
x86/cpu_hotplug: Update the link of cpu_hotplug.rst
x86/smpboot: Make the check code more clear in prefill_possible_map()
acpi/processor: Fix the return value of acpi_processor_ids_walk()
acpi/processor: Make the acpi_duplicate_processor_id() static
Documentation/00-INDEX | 2 -
Documentation/admin-guide/kernel-parameters.txt | 5 ++
Documentation/cputopology.txt | 10 ++--
Documentation/x86/x86_64/cpu-hotplug-spec | 2 +-
arch/x86/kernel/smpboot.c | 31 +++++++-----
drivers/acpi/acpi_processor.c | 66 ++++++++++++-------------
include/linux/acpi.h | 3 --
7 files changed, 62 insertions(+), 57 deletions(-)
--
2.14.3
Kernel uses the possible_cpus in command line to reset the possible_cpus bits
in cpu_possible_map. It doesn't be recorded in the kernel-parameters.txt
Add its description in this document, also replace the wrong option additional_cpus
with possible_cpus in cpu-gotplug-spec.
Signed-off-by: Dou Liyang <[email protected]>
---
Documentation/admin-guide/kernel-parameters.txt | 5 +++++
Documentation/x86/x86_64/cpu-hotplug-spec | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..34f8a5f04e63 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2832,6 +2832,11 @@
variables need be pre-allocated for later physical cpu
hot plugging.
+ possible_cpus= [s390,x86_64] Use this to set hotpluggable cpus.
+ This option sets possible_cpus bits in cpu_possible_map.
+ Thus keeping the numbers of bits set constant even if
+ the machine gets rebooted.
+
nr_uarts= [SERIAL] maximum number of UARTs to be registered.
numa_balancing= [KNL,X86] Enable or disable automatic NUMA balancing.
diff --git a/Documentation/x86/x86_64/cpu-hotplug-spec b/Documentation/x86/x86_64/cpu-hotplug-spec
index 3c23e0587db3..d218bc0bdaaa 100644
--- a/Documentation/x86/x86_64/cpu-hotplug-spec
+++ b/Documentation/x86/x86_64/cpu-hotplug-spec
@@ -16,6 +16,6 @@ it should have its LAPIC Enabled bit set to 0. Linux will use the number
of disabled LAPICs to compute the maximum number of future CPUs.
In the worst case the user can overwrite this choice using a command line
-option (additional_cpus=...), but it is recommended to supply the correct
+option (possible_cpus=...), but it is recommended to supply the correct
number (or a reasonable approximation of it, with erring towards more not less)
in the MADT to avoid manual configuration.
--
2.14.3
ACPI driver should make sure all the processor IDs in their ACPI Namespace
are unique for CPU hotplug. the driver performs a depth-first walk of the
namespace tree and calls the acpi_processor_ids_walk().
But, the acpi_processor_ids_walk() will return true if one processor is
checked, that cause the walk break after walking pass the first processor.
Repace the value with AE_OK which is the standard acpi_status value.
Fixes 8c8cb30f49b8 ("acpi/processor: Implement DEVICE operator for processor enumeration")
Signed-off-by: Dou Liyang <[email protected]>
---
drivers/acpi/acpi_processor.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 449d86d39965..db5bdb59639c 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -663,11 +663,11 @@ static acpi_status __init acpi_processor_ids_walk(acpi_handle handle,
}
processor_validated_ids_update(uid);
- return true;
+ return AE_OK;
err:
acpi_handle_info(handle, "Invalid processor object\n");
- return false;
+ return AE_OK;
}
--
2.14.3
The acpi_duplicate_processor_id() is only called in acpi_processor_get_info(),
So move it in front of acpi_processor_get_info() and make it static.
Signed-off-by: Dou Liyang <[email protected]>
---
drivers/acpi/acpi_processor.c | 62 +++++++++++++++++++++----------------------
include/linux/acpi.h | 3 ---
2 files changed, 31 insertions(+), 34 deletions(-)
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index db5bdb59639c..03ec7690710c 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -229,6 +229,37 @@ static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
}
#endif /* CONFIG_ACPI_HOTPLUG_CPU */
+/* The number of the unique processor IDs */
+static int nr_unique_ids __initdata;
+
+/* The number of the duplicate processor IDs */
+static int nr_duplicate_ids;
+
+/* Used to store the unique processor IDs */
+static int unique_processor_ids[] __initdata = {
+ [0 ... NR_CPUS - 1] = -1,
+};
+
+/* Used to store the duplicate processor IDs */
+static int duplicate_processor_ids[] = {
+ [0 ... NR_CPUS - 1] = -1,
+};
+
+static bool acpi_duplicate_processor_id(int proc_id)
+{
+ int i;
+
+ /*
+ * Compare the proc_id with duplicate IDs, if the proc_id is already
+ * in the duplicate IDs, return true, otherwise, return false.
+ */
+ for (i = 0; i < nr_duplicate_ids; i++) {
+ if (duplicate_processor_ids[i] == proc_id)
+ return true;
+ }
+ return false;
+}
+
static int acpi_processor_get_info(struct acpi_device *device)
{
union acpi_object object = { 0 };
@@ -579,22 +610,6 @@ static struct acpi_scan_handler processor_container_handler = {
.attach = acpi_processor_container_attach,
};
-/* The number of the unique processor IDs */
-static int nr_unique_ids __initdata;
-
-/* The number of the duplicate processor IDs */
-static int nr_duplicate_ids;
-
-/* Used to store the unique processor IDs */
-static int unique_processor_ids[] __initdata = {
- [0 ... NR_CPUS - 1] = -1,
-};
-
-/* Used to store the duplicate processor IDs */
-static int duplicate_processor_ids[] = {
- [0 ... NR_CPUS - 1] = -1,
-};
-
static void __init processor_validated_ids_update(int proc_id)
{
int i;
@@ -682,21 +697,6 @@ static void __init acpi_processor_check_duplicates(void)
NULL, NULL);
}
-bool acpi_duplicate_processor_id(int proc_id)
-{
- int i;
-
- /*
- * compare the proc_id with duplicate IDs, if the proc_id is already
- * in the duplicate IDs, return true, otherwise, return false.
- */
- for (i = 0; i < nr_duplicate_ids; i++) {
- if (duplicate_processor_ids[i] == proc_id)
- return true;
- }
- return false;
-}
-
void __init acpi_processor_init(void)
{
acpi_processor_check_duplicates();
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 968173ec2726..dd4591dc1eb3 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -285,9 +285,6 @@ static inline bool invalid_phys_cpuid(phys_cpuid_t phys_id)
return phys_id == PHYS_CPUID_INVALID;
}
-/* Validate the processor object's proc_id */
-bool acpi_duplicate_processor_id(int proc_id);
-
#ifdef CONFIG_ACPI_HOTPLUG_CPU
/* Arch dependent functions for cpu hotplug support */
int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, u32 acpi_id,
--
2.14.3
In prefill_possible_map(), Kernel need to get the number of possible CPUs
to reset cpu_possible_map. The number is related to the options:
-nosmp, maxcpus, possible_cpus, nr_cpus
... which need to be checked.
Currentlly, the check code mixed these options together in confusion and
hard to follow.
Isolate them, and check them linearly.
No functionary change, Prepare for cutting the number of possible CPUs
Signed-off-by: Dou Liyang <[email protected]>
---
Situations:
setup_possible_cpus == -1 | setup_max_cpus == 0 | CONFIG_HOTPLUG_CPU == y |
yes | yes | yes |
no | no | no |
Test cases:
Cases | the number of possible cpus
| (the same with or w/o this patch)
case 1: no | no | no | --> min (setup_possible_cpus, nr_cpu_ids, setup_max_cpus)
case 2: no | no | yes| --> min (setup_possible_cpus, nr_cpu_ids)
case 3: no | yes | no | --> 1
case 4: no | yes | yes| --> 1
case 5: yes | no | no | --> min (num_processors, nr_cpu_ids, setup_max_cpus)
case 6: yes | no | yes| --> min (num_processors + disabled_cpus, nr_cpu_ids)
case 7: yes | yes | no | --> 1
case 8: yes | yes | yes| --> 1
arch/x86/kernel/smpboot.c | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ff99e2b6fc54..2fdda8567bf9 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1334,7 +1334,7 @@ early_param("possible_cpus", _setup_possible_cpus);
* We do this because additional CPUs waste a lot of memory.
* -AK
*/
-__init void prefill_possible_map(void)
+void __init prefill_possible_map(void)
{
int i, possible;
@@ -1356,18 +1356,22 @@ __init void prefill_possible_map(void)
num_processors = 1;
}
- i = setup_max_cpus ?: 1;
+ /* The SMP kernel could be acted as an UP kernel via nosmp/maxcpus=0 */
+ if (!setup_max_cpus) {
+ possible = 1;
+ total_cpus = num_processors + disabled_cpus;
+ goto set_cpu_possible_mask;
+ }
+
+ /* Possible CPUs could be overwrited via possible_cpus= */
if (setup_possible_cpus == -1) {
possible = num_processors;
#ifdef CONFIG_HOTPLUG_CPU
- if (setup_max_cpus)
- possible += disabled_cpus;
-#else
- if (possible > i)
- possible = i;
+ possible += disabled_cpus;
#endif
- } else
+ } else {
possible = setup_possible_cpus;
+ }
total_cpus = max_t(int, possible, num_processors + disabled_cpus);
@@ -1378,15 +1382,16 @@ __init void prefill_possible_map(void)
possible = nr_cpu_ids;
}
-#ifdef CONFIG_HOTPLUG_CPU
- if (!setup_max_cpus)
-#endif
- if (possible > i) {
+#ifndef CONFIG_HOTPLUG_CPU
+ /* Possible CPUs could be reduced via max_cpus= */
+ if (possible > setup_max_cpus) {
pr_warn("%d Processors exceeds max_cpus limit of %u\n",
possible, setup_max_cpus);
- possible = i;
+ possible = setup_max_cpus;
}
+#endif
+set_cpu_possible_mask:
nr_cpu_ids = possible;
pr_info("Allowing %d CPUs, %d hotplug CPUs\n",
--
2.14.3
The original cpu_hotplug.txt documents describing CPU hotplug support in
Linux kernel. it was moved in to core-api/ and renamed cpu_hotplug.rst.
Update it's link in other documents
Fixes: ff58fa7f556c ("Documentation: Update CPU hotplug and move it to core-api")
Signed-off-by: Dou Liyang <[email protected]>
---
Documentation/00-INDEX | 2 --
Documentation/cputopology.txt | 10 +++++-----
2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX
index 7f3a0728ccf2..3773c67ea9e5 100644
--- a/Documentation/00-INDEX
+++ b/Documentation/00-INDEX
@@ -104,8 +104,6 @@ core-api/
- documentation on kernel core components.
cpu-freq/
- info on CPU frequency and voltage scaling.
-cpu-hotplug.txt
- - document describing CPU hotplug support in the Linux kernel.
cpu-load.txt
- document describing how CPU load statistics are collected.
cpuidle/
diff --git a/Documentation/cputopology.txt b/Documentation/cputopology.txt
index c6e7e9196a8b..e05b0879fe91 100644
--- a/Documentation/cputopology.txt
+++ b/Documentation/cputopology.txt
@@ -117,9 +117,9 @@ source for the output is in brackets ("[]").
[NR_CPUS-1]
offline: CPUs that are not online because they have been
- HOTPLUGGED off (see cpu-hotplug.txt) or exceed the limit
- of CPUs allowed by the kernel configuration (kernel_max
- above). [~cpu_online_mask + cpus >= NR_CPUS]
+ HOTPLUGGED off (see core-api/cpu_hotplug.rst) or exceed
+ the limit of CPUs allowed by the kernel configuration
+ (kernel_max above). [~cpu_online_mask + cpus >= NR_CPUS]
online: CPUs that are online and being scheduled [cpu_online_mask]
@@ -155,5 +155,5 @@ online.)::
possible: 0-127
present: 0-3
-See cpu-hotplug.txt for the possible_cpus=NUM kernel start parameter
-as well as more information on the various cpumasks.
+See core-api/cpu_hotplug.rst for the possible_cpus=NUM kernel start
+parameter as well as more information on the various cpumasks.
--
2.14.3
On Tue, Mar 20, 2018 at 07:04:28PM +0800, Dou Liyang wrote:
> + possible_cpus= [s390,x86_64] Use this to set hotpluggable cpus.
> + This option sets possible_cpus bits in cpu_possible_map.
> + Thus keeping the numbers of bits set constant even if
> + the machine gets rebooted.
That description, esp. the last sentence, doesn't make any kind of sense
to me. What?
On Tue, Mar 20, 2018 at 07:04:30PM +0800, Dou Liyang wrote:
> case 1: no | no | no | --> min (setup_possible_cpus, nr_cpu_ids, setup_max_cpus)
> case 2: no | no | yes| --> min (setup_possible_cpus, nr_cpu_ids)
> case 3: no | yes | no | --> 1
> case 4: no | yes | yes| --> 1
> case 5: yes | no | no | --> min (num_processors, nr_cpu_ids, setup_max_cpus)
> case 6: yes | no | yes| --> min (num_processors + disabled_cpus, nr_cpu_ids)
> case 7: yes | yes | no | --> 1
> case 8: yes | yes | yes| --> 1
The case number is off by one ;-)
Hi Peter,
At 03/20/2018 08:37 PM, Peter Zijlstra wrote:
> On Tue, Mar 20, 2018 at 07:04:28PM +0800, Dou Liyang wrote:
>
>> + possible_cpus= [s390,x86_64] Use this to set hotpluggable cpus.
>> + This option sets possible_cpus bits in cpu_possible_map.
>> + Thus keeping the numbers of bits set constant even if
>> + the machine gets rebooted.
>
> That description, esp. the last sentence, doesn't make any kind of sense
> to me. What?
>
Ah, sure enough, I can't be lazy. :-) I stole that from the commit
3b11ce7f542e ("x86: use possible_cpus=NUM to extend the possible cpus
allowed")
How about:
possible_cpus= [s390,x86_64] Set the number of possible CPUs which
are determined by the ACPI tables MADT or mptables by
default. possible_cpus=n : n >= 1 enforces the possible
number to be 'n'.
While nr_cpus is also be set: nr_cpus=m, choice the
minimum one for the number of possible CPUs.
Thank,
dou
Hi Peter,
At 03/20/2018 08:39 PM, Peter Zijlstra wrote:
> On Tue, Mar 20, 2018 at 07:04:30PM +0800, Dou Liyang wrote:
>> case 1: no | no | no | --> min (setup_possible_cpus, nr_cpu_ids, setup_max_cpus)
>> case 2: no | no | yes| --> min (setup_possible_cpus, nr_cpu_ids)
>> case 3: no | yes | no | --> 1
>> case 4: no | yes | yes| --> 1
>> case 5: yes | no | no | --> min (num_processors, nr_cpu_ids, setup_max_cpus)
>> case 6: yes | no | yes| --> min (num_processors + disabled_cpus, nr_cpu_ids)
>> case 7: yes | yes | no | --> 1
>> case 8: yes | yes | yes| --> 1
>
> The case number is off by one ;-)
>
I got it! ;-??
Thanks
dou
>
>
On Wed, Mar 21, 2018 at 01:33:24PM +0800, Dou Liyang wrote:
> How about:
>
> possible_cpus= [s390,x86_64] Set the number of possible CPUs which
> are determined by the ACPI tables MADT or mptables by
> default. possible_cpus=n : n >= 1 enforces the possible
> number to be 'n'.
> While nr_cpus is also be set: nr_cpus=m, choice the
> minimum one for the number of possible CPUs.
So what is the exact difference between possible_cpus and nr_cpus ? I
konw maxcpus= limits the number of CPUs we bring up, and possible_cpus
limits the possible_map, but I'm not entirely sure what nr_cpus does
here.
Hi Peter,
At 03/21/2018 05:08 PM, Peter Zijlstra wrote:
> On Wed, Mar 21, 2018 at 01:33:24PM +0800, Dou Liyang wrote:
>> How about:
>>
>> possible_cpus= [s390,x86_64] Set the number of possible CPUs which
>> are determined by the ACPI tables MADT or mptables by
>> default. possible_cpus=n : n >= 1 enforces the possible
>> number to be 'n'.
>> While nr_cpus is also be set: nr_cpus=m, choice the
>> minimum one for the number of possible CPUs.
>
> So what is the exact difference between possible_cpus and nr_cpus ? I
the possible_cpus= can reset the number of possible CPUs, even bigger
than 'num_processors + disabled_cpus', But nr_cpus= can't.
> konw maxcpus= limits the number of CPUs we bring up, and possible_cpus
> limits the possible_map, but I'm not entirely sure what nr_cpus does
> here.
>
nr_cpus can limited the maximum CPUs that the kernel could support.
Here is a double check in case of using them at the same time, even if I
think just using possible_cpus= is enough. :-)
Thanks,
dou.
>
>
On Wed, 21 Mar 2018, Peter Zijlstra wrote:
> On Wed, Mar 21, 2018 at 01:33:24PM +0800, Dou Liyang wrote:
> > How about:
> >
> > possible_cpus= [s390,x86_64] Set the number of possible CPUs which
> > are determined by the ACPI tables MADT or mptables by
> > default. possible_cpus=n : n >= 1 enforces the possible
> > number to be 'n'.
> > While nr_cpus is also be set: nr_cpus=m, choice the
> > minimum one for the number of possible CPUs.
>
> So what is the exact difference between possible_cpus and nr_cpus ? I
> konw maxcpus= limits the number of CPUs we bring up, and possible_cpus
> limits the possible_map, but I'm not entirely sure what nr_cpus does
> here.
nr_cpus limits the number of CPUs the kernel will handle. Think of it as a
boot time override of NR_CPUs.
Way too many commandline switches though.
Thanks,
tglx
At 03/21/2018 05:34 PM, Dou Liyang wrote:
> Hi Peter,
>
> At 03/21/2018 05:08 PM, Peter Zijlstra wrote:
>> On Wed, Mar 21, 2018 at 01:33:24PM +0800, Dou Liyang wrote:
>>> How about:
>>>
>>> possible_cpus= [s390,x86_64] Set the number of possible CPUs which
>>> are determined by the ACPI tables MADT or mptables by
>>> default. possible_cpus=n : n >= 1 enforces the possible
>>> number to be 'n'.
>>> While nr_cpus is also be set: nr_cpus=m, choice the
>>> minimum one for the number of possible CPUs.
>>
>> So what is the exact difference between possible_cpus and nr_cpus ? I
>
> the possible_cpus= can reset the number of possible CPUs, even bigger
> than 'num_processors + disabled_cpus', But nr_cpus= can't.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
the maximum number kernel gets from ACPI/mptables, no matter what
number nr_cpus= is, the number of possible CPUs will not bigger than it.
>
>> konw maxcpus= limits the number of CPUs we bring up, and possible_cpus
>> limits the possible_map, but I'm not entirely sure what nr_cpus does
>> here.
>>
>
> nr_cpus can limited the maximum CPUs that the kernel could support.
>
> Here is a double check in case of using them at the same time, even if I
> think just using possible_cpus= is enough. :-)
>
> Thanks,
> dou.
>>
>>
On Tue, 20 Mar 2018, Dou Liyang wrote:
> ACPI driver should make sure all the processor IDs in their ACPI Namespace
> are unique for CPU hotplug. the driver performs a depth-first walk of the
> namespace tree and calls the acpi_processor_ids_walk().
>
> But, the acpi_processor_ids_walk() will return true if one processor is
> checked, that cause the walk break after walking pass the first processor.
>
> Repace the value with AE_OK which is the standard acpi_status value.
>
> Fixes 8c8cb30f49b8 ("acpi/processor: Implement DEVICE operator for processor enumeration")
>
> Signed-off-by: Dou Liyang <[email protected]>
> ---
> drivers/acpi/acpi_processor.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 449d86d39965..db5bdb59639c 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -663,11 +663,11 @@ static acpi_status __init acpi_processor_ids_walk(acpi_handle handle,
> }
>
> processor_validated_ids_update(uid);
> - return true;
> + return AE_OK;
>
> err:
> acpi_handle_info(handle, "Invalid processor object\n");
> - return false;
> + return AE_OK;
I'm not sure whether this is the right return value here. Rafael?
Thanks,
tglx
At 05/19/2018 11:06 PM, Thomas Gleixner wrote:
> On Tue, 20 Mar 2018, Dou Liyang wrote:
>
>> ACPI driver should make sure all the processor IDs in their ACPI Namespace
>> are unique for CPU hotplug. the driver performs a depth-first walk of the
>> namespace tree and calls the acpi_processor_ids_walk().
>>
>> But, the acpi_processor_ids_walk() will return true if one processor is
>> checked, that cause the walk break after walking pass the first processor.
>>
>> Repace the value with AE_OK which is the standard acpi_status value.
>>
>> Fixes 8c8cb30f49b8 ("acpi/processor: Implement DEVICE operator for processor enumeration")
>>
>> Signed-off-by: Dou Liyang <[email protected]>
>> ---
>> drivers/acpi/acpi_processor.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>> index 449d86d39965..db5bdb59639c 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -663,11 +663,11 @@ static acpi_status __init (acpi_handle handle,
>> }
>>
>> processor_validated_ids_update(uid);
>> - return true;
>> + return AE_OK;
>>
>> err:
>> acpi_handle_info(handle, "Invalid processor object\n");
>> - return false;
>> + return AE_OK;
>
> I'm not sure whether this is the right return value here. Rafael?
>
Hi, Thomas, Rafael,
Yes, I used AE_OK to make sure it can skip the invalid objects and
continue to do the following other objects, I'm also not sure.
For this bug, recently, I sent another patch to remove this check code
away.
https://lkml.org/lkml/2018/5/17/320
IMO, the duplicate IDs can be avoid by the other code
if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) ---- 1)
As the mapping of cpu_id(pr->id) and processor_id is fixed, when
hot-plugging a physical CPU, if its processor_id is duplicated with the
present, the above condition 1) will be 0, and Linux will do not add
this CPU.
And, when every time the system starts, this code will be executed, it
will waste more time with the increase in the number of CPU.
So I prefer to remove this code.
Thanks,
dou
At 05/22/2018 09:47 AM, Dou Liyang wrote:
>
>
> At 05/19/2018 11:06 PM, Thomas Gleixner wrote:
>> On Tue, 20 Mar 2018, Dou Liyang wrote:
>>
>>> ACPI driver should make sure all the processor IDs in their ACPI
>>> Namespace
>>> are unique for CPU hotplug. the driver performs a depth-first walk of
>>> the
>>> namespace tree and calls the acpi_processor_ids_walk().
>>>
>>> But, the acpi_processor_ids_walk() will return true if one processor is
>>> checked, that cause the walk break after walking pass the first
>>> processor.
>>>
>>> Repace the value with AE_OK which is the standard acpi_status value.
>>>
>>> Fixes 8c8cb30f49b8 ("acpi/processor: Implement DEVICE operator for
>>> processor enumeration")
>>>
>>> Signed-off-by: Dou Liyang <[email protected]>
>>> ---
>>> drivers/acpi/acpi_processor.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/acpi/acpi_processor.c
>>> b/drivers/acpi/acpi_processor.c
>>> index 449d86d39965..db5bdb59639c 100644
>>> --- a/drivers/acpi/acpi_processor.c
>>> +++ b/drivers/acpi/acpi_processor.c
>>> @@ -663,11 +663,11 @@ static acpi_status __init (acpi_handle handle,
>>> }
>>> processor_validated_ids_update(uid);
>>> - return true;
>>> + return AE_OK;
>>> err:
>>> acpi_handle_info(handle, "Invalid processor object\n");
>>> - return false;
>>> + return AE_OK;
>>
>> I'm not sure whether this is the right return value here. Rafael?
>>
+Cc Rafael's common used email address.
I am sorry, I created the cc list using ./script/get_maintainers.pl ...
and didn't check it.
Thanks,
dou
> Hi, Thomas, Rafael,
>
> Yes, I used AE_OK to make sure it can skip the invalid objects and
> continue to do the following other objects, I'm also not sure.
>
> For this bug, recently, I sent another patch to remove this check code
> away.
>
> https://lkml.org/lkml/2018/5/17/320
>
> IMO, the duplicate IDs can be avoid by the other code
>
> if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) ---- 1)
>
> As the mapping of cpu_id(pr->id) and processor_id is fixed, when
> hot-plugging a physical CPU, if its processor_id is duplicated with the
> present, the above condition 1) will be 0, and Linux will do not add
> this CPU.
>
> And, when every time the system starts, this code will be executed, it
> will waste more time with the increase in the number of CPU.
>
> So I prefer to remove this code.
>
> Thanks,
> dou
>
>
>
On Wed, May 23, 2018 at 3:34 AM, Dou Liyang <[email protected]> wrote:
> At 05/22/2018 09:47 AM, Dou Liyang wrote:
>>
>>
>>
>> At 05/19/2018 11:06 PM, Thomas Gleixner wrote:
>>>
>>> On Tue, 20 Mar 2018, Dou Liyang wrote:
>>>
>>>> ACPI driver should make sure all the processor IDs in their ACPI
>>>> Namespace
>>>> are unique for CPU hotplug. the driver performs a depth-first walk of
>>>> the
>>>> namespace tree and calls the acpi_processor_ids_walk().
>>>>
>>>> But, the acpi_processor_ids_walk() will return true if one processor is
>>>> checked, that cause the walk break after walking pass the first
>>>> processor.
>>>>
>>>> Repace the value with AE_OK which is the standard acpi_status value.
>>>>
>>>> Fixes 8c8cb30f49b8 ("acpi/processor: Implement DEVICE operator for
>>>> processor enumeration")
>>>>
>>>> Signed-off-by: Dou Liyang <[email protected]>
>>>> ---
>>>> drivers/acpi/acpi_processor.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/acpi_processor.c
>>>> b/drivers/acpi/acpi_processor.c
>>>> index 449d86d39965..db5bdb59639c 100644
>>>> --- a/drivers/acpi/acpi_processor.c
>>>> +++ b/drivers/acpi/acpi_processor.c
>>>> @@ -663,11 +663,11 @@ static acpi_status __init (acpi_handle handle,
>>>> }
>>>> processor_validated_ids_update(uid);
>>>> - return true;
>>>> + return AE_OK;
>>>> err:
>>>> acpi_handle_info(handle, "Invalid processor object\n");
>>>> - return false;
>>>> + return AE_OK;
>>>
>>>
>>> I'm not sure whether this is the right return value here. Rafael?
>>>
>
> +Cc Rafael's common used email address.
>
> I am sorry, I created the cc list using ./script/get_maintainers.pl ...
> and didn't check it.
No worries, I saw your messages, but thanks!