Add disable_cpu_apicid kernel parameter. To use this kernel parameter,
specify an initial APIC ID of the corresponding CPU you want to
disable.
This is mostly used for the kdump 2nd kernel to disable BSP to wake up
multiple CPUs without causing system reset or hang due to sending INIT
from AP to BSP.
Kdump users first figure out initial APIC ID of the BSP, CPU0 in the
1st kernel, for example from /proc/cpuinfo and then set up this kernel
parameter for the 2nd kernel using the obtained APIC ID.
However, doing this procedure at each boot time manually is awkward,
which should be automatically done by user-land service scripts, for
example, kexec-tools on fedora/RHEL distributions.
This design is more flexible than disabling BSP in kernel boot time
automatically in that in kernel boot time we have no choice but
referring to ACPI/MP table to obtain initial APIC ID for BSP, meaning
that the method is not applicable to the systems without such BIOS
tables.
One assumption behind this design is that users get initial APIC ID of
the BSP in still healthy state and so BSP is uniquely kept in
CPU0. Thus, through the kernel parameter, only one initial APIC ID can
be specified.
In a comparison with disabled_cpu_apicid, we use read_apic_id(), not
boot_cpu_physical_apicid, because on some platforms, the variable is
modified to the apicid reported as BSP through MP table and this
function is executed with the temporarily modified
boot_cpu_physical_apicid. As a result, disabled_cpu_apicid kernel
parameter doesn't work well for apicids of APs.
Fixing the wrong handling of boot_cpu_physical_apicid requires some
reviews and tests beyond some platforms and it could take some
time. The fix here is a kind of workaround to focus on the main topic
of this patch.
Signed-off-by: HATAYAMA Daisuke <[email protected]>
---
Documentation/kernel-parameters.txt | 9 ++++++
arch/x86/kernel/apic/apic.c | 49 +++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 50680a5..4e5528c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -774,6 +774,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
disable= [IPV6]
See Documentation/networking/ipv6.txt.
+ disable_cpu_apicid= [X86,APIC,SMP]
+ Format: <int>
+ The number of initial APIC ID for the
+ corresponding CPU to be disabled at boot,
+ mostly used for the kdump 2nd kernel to
+ disable BSP to wake up multiple CPUs without
+ causing system reset or hang due to sending
+ INIT from AP to BSP.
+
disable_ddw [PPC/PSERIES]
Disable Dynamic DMA Window support. Use this if
to workaround buggy firmware.
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index d278736..6c0b7d5 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -75,6 +75,13 @@ unsigned int max_physical_apicid;
physid_mask_t phys_cpu_present_map;
/*
+ * Processor to be disabled specified by kernel parameter
+ * disable_cpu_apicid=<int>, mostly used for the kdump 2nd kernel to
+ * avoid undefined behaviour caused by sending INIT from AP to BSP.
+ */
+unsigned int disabled_cpu_apicid = BAD_APICID;
+
+/*
* Map cpu index to physical APIC ID
*/
DEFINE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_cpu_to_apicid, BAD_APICID);
@@ -2115,6 +2122,39 @@ int generic_processor_info(int apicid, int version)
phys_cpu_present_map);
/*
+ * boot_cpu_physical_apicid is designed to have the apicid
+ * returned by read_apic_id(), i.e, the apicid of the
+ * currently booting-up processor. However, on some platforms,
+ * it is temporarilly modified by the apicid reported as BSP
+ * through MP table. Concretely:
+ *
+ * - arch/x86/kernel/mpparse.c: MP_processor_info()
+ * - arch/x86/mm/amdtopology.c: amd_numa_init()
+ * - arch/x86/platform/visws/visws_quirks.c: MP_processor_info()
+ *
+ * This function is executed with the modified
+ * boot_cpu_physical_apicid. So, disabled_cpu_apicid kernel
+ * parameter doesn't work to disable APs on kdump 2nd kernel.
+ *
+ * Since fixing handling of boot_cpu_physical_apicid requires
+ * another discussion and tests on each platform, we leave it
+ * for now and here we use read_apic_id() directly in this
+ * function, generic_processor_info().
+ */
+ if (disabled_cpu_apicid != BAD_APICID &&
+ disabled_cpu_apicid != read_apic_id() &&
+ disabled_cpu_apicid == apicid) {
+ int thiscpu = num_processors + disabled_cpus;
+
+ pr_warning("ACPI: Disabling requested cpu."
+ " Processor %d/0x%x ignored.\n",
+ thiscpu, apicid);
+
+ disabled_cpus++;
+ return -ENODEV;
+ }
+
+ /*
* If boot cpu has not been detected yet, then only allow upto
* nr_cpu_ids - 1 processors and keep one slot free for boot cpu
*/
@@ -2592,3 +2632,12 @@ static int __init lapic_insert_resource(void)
* that is using request_resource
*/
late_initcall(lapic_insert_resource);
+
+static int __init apic_set_disabled_cpu_apicid(char *arg)
+{
+ if (!arg || !get_option(&arg, &disabled_cpu_apicid))
+ return -EINVAL;
+
+ return 0;
+}
+early_param("disable_cpu_apicid", apic_set_disabled_cpu_apicid);
On Wed, Jan 15, 2014 at 03:44:58PM +0900, HATAYAMA Daisuke wrote:
> Add disable_cpu_apicid kernel parameter. To use this kernel parameter,
> specify an initial APIC ID of the corresponding CPU you want to
> disable.
>
> This is mostly used for the kdump 2nd kernel to disable BSP to wake up
> multiple CPUs without causing system reset or hang due to sending INIT
> from AP to BSP.
>
> Kdump users first figure out initial APIC ID of the BSP, CPU0 in the
> 1st kernel, for example from /proc/cpuinfo and then set up this kernel
> parameter for the 2nd kernel using the obtained APIC ID.
>
> However, doing this procedure at each boot time manually is awkward,
> which should be automatically done by user-land service scripts, for
> example, kexec-tools on fedora/RHEL distributions.
>
> This design is more flexible than disabling BSP in kernel boot time
> automatically in that in kernel boot time we have no choice but
> referring to ACPI/MP table to obtain initial APIC ID for BSP, meaning
> that the method is not applicable to the systems without such BIOS
> tables.
>
> One assumption behind this design is that users get initial APIC ID of
> the BSP in still healthy state and so BSP is uniquely kept in
> CPU0. Thus, through the kernel parameter, only one initial APIC ID can
> be specified.
>
> In a comparison with disabled_cpu_apicid, we use read_apic_id(), not
> boot_cpu_physical_apicid, because on some platforms, the variable is
> modified to the apicid reported as BSP through MP table and this
> function is executed with the temporarily modified
> boot_cpu_physical_apicid. As a result, disabled_cpu_apicid kernel
> parameter doesn't work well for apicids of APs.
>
> Fixing the wrong handling of boot_cpu_physical_apicid requires some
> reviews and tests beyond some platforms and it could take some
> time. The fix here is a kind of workaround to focus on the main topic
> of this patch.
>
> Signed-off-by: HATAYAMA Daisuke <[email protected]>
I think this is a reasonable approach to solve the issue. Use a command
line to not bring up specific cpu in second kernel which can create
problems.
Acked-by: Vivek Goyal <[email protected]>
hpa, I know you are not excited about this approach. If you made up your
mind that this appoarch is not worth pursuing, please do suggest what
would you like to see and we can give that a try.
We want to solve this problem as on large memory machines saving dump can
take lot of time and we want to bring up multiple cpus and speed up
compression and save on dump time.
Thanks
Vivek
On 01/15/2014 09:05 AM, Vivek Goyal wrote:
>
> I think this is a reasonable approach to solve the issue. Use a command
> line to not bring up specific cpu in second kernel which can create
> problems.
>
> Acked-by: Vivek Goyal <[email protected]>
>
> hpa, I know you are not excited about this approach. If you made up your
> mind that this appoarch is not worth pursuing, please do suggest what
> would you like to see and we can give that a try.
>
> We want to solve this problem as on large memory machines saving dump can
> take lot of time and we want to bring up multiple cpus and speed up
> compression and save on dump time.
>
I'm not excited about kdump's reliance on the command line, since it
seems to be a neverending source of trouble, simply because the command
line is fundamentally intended as a human interface. However, this
seems relatively harmless in comparison with everything else and I am
much happier with saving the ID in the first kernel rather than trying
to guess if a currently-downed CPU is the BSP.
-hpa
On Wed, Jan 15, 2014 at 09:26:14AM -0800, H. Peter Anvin wrote:
> On 01/15/2014 09:05 AM, Vivek Goyal wrote:
> >
> > I think this is a reasonable approach to solve the issue. Use a command
> > line to not bring up specific cpu in second kernel which can create
> > problems.
> >
> > Acked-by: Vivek Goyal <[email protected]>
> >
> > hpa, I know you are not excited about this approach. If you made up your
> > mind that this appoarch is not worth pursuing, please do suggest what
> > would you like to see and we can give that a try.
> >
> > We want to solve this problem as on large memory machines saving dump can
> > take lot of time and we want to bring up multiple cpus and speed up
> > compression and save on dump time.
> >
>
> I'm not excited about kdump's reliance on the command line, since it
> seems to be a neverending source of trouble, simply because the command
> line is fundamentally intended as a human interface.
So in general, what are the alternatives? Either we figure out that kernel
is booting as kdump kernel and do things differently. That seems even
worse as what do we want in kdump kernel will change over a period of
time.
Other thing is that pass more information in bootparams. But that does
not seem much different than command line to me.
> However, this
> seems relatively harmless in comparison with everything else and I am
> much happier with saving the ID in the first kernel rather than trying
> to guess if a currently-downed CPU is the BSP.
So looks like you are alright with this patch. Can you please queue it
up in your tree.
Thanks
Vivek
On 01/15/2014 09:47 AM, Vivek Goyal wrote:
> On Wed, Jan 15, 2014 at 09:26:14AM -0800, H. Peter Anvin wrote:
>> On 01/15/2014 09:05 AM, Vivek Goyal wrote:
>>>
>>> I think this is a reasonable approach to solve the issue. Use a command
>>> line to not bring up specific cpu in second kernel which can create
>>> problems.
>>>
>>> Acked-by: Vivek Goyal <[email protected]>
>>>
>>> hpa, I know you are not excited about this approach. If you made up your
>>> mind that this appoarch is not worth pursuing, please do suggest what
>>> would you like to see and we can give that a try.
>>>
>>> We want to solve this problem as on large memory machines saving dump can
>>> take lot of time and we want to bring up multiple cpus and speed up
>>> compression and save on dump time.
>>>
>>
>> I'm not excited about kdump's reliance on the command line, since it
>> seems to be a neverending source of trouble, simply because the command
>> line is fundamentally intended as a human interface.
>
> So in general, what are the alternatives? Either we figure out that kernel
> is booting as kdump kernel and do things differently. That seems even
> worse as what do we want in kdump kernel will change over a period of
> time.
>
> Other thing is that pass more information in bootparams. But that does
> not seem much different than command line to me.
>
It is the commingling of semantics that is the problem. Command line
options are generally imperative, "do this". What you want in the kdump
situation, as you yourself state above, is get a description of the
current situation and let the kdump side choose the action to take.
As a transport mechanism the command line suffers from limited size and
that you have to share it with an arbitrary amount of user-provided
options that may or may not be essential.
>> However, this
>> seems relatively harmless in comparison with everything else and I am
>> much happier with saving the ID in the first kernel rather than trying
>> to guess if a currently-downed CPU is the BSP.
>
> So looks like you are alright with this patch. Can you please queue it
> up in your tree.
>
In process.
-hpa
Commit-ID: 151e0c7de616310f95393d9306903900fcd8b277
Gitweb: http://git.kernel.org/tip/151e0c7de616310f95393d9306903900fcd8b277
Author: HATAYAMA Daisuke <[email protected]>
AuthorDate: Wed, 15 Jan 2014 15:44:58 +0900
Committer: H. Peter Anvin <[email protected]>
CommitDate: Wed, 15 Jan 2014 09:19:20 -0800
x86, apic, kexec: Add disable_cpu_apicid kernel parameter
Add disable_cpu_apicid kernel parameter. To use this kernel parameter,
specify an initial APIC ID of the corresponding CPU you want to
disable.
This is mostly used for the kdump 2nd kernel to disable BSP to wake up
multiple CPUs without causing system reset or hang due to sending INIT
from AP to BSP.
Kdump users first figure out initial APIC ID of the BSP, CPU0 in the
1st kernel, for example from /proc/cpuinfo and then set up this kernel
parameter for the 2nd kernel using the obtained APIC ID.
However, doing this procedure at each boot time manually is awkward,
which should be automatically done by user-land service scripts, for
example, kexec-tools on fedora/RHEL distributions.
This design is more flexible than disabling BSP in kernel boot time
automatically in that in kernel boot time we have no choice but
referring to ACPI/MP table to obtain initial APIC ID for BSP, meaning
that the method is not applicable to the systems without such BIOS
tables.
One assumption behind this design is that users get initial APIC ID of
the BSP in still healthy state and so BSP is uniquely kept in
CPU0. Thus, through the kernel parameter, only one initial APIC ID can
be specified.
In a comparison with disabled_cpu_apicid, we use read_apic_id(), not
boot_cpu_physical_apicid, because on some platforms, the variable is
modified to the apicid reported as BSP through MP table and this
function is executed with the temporarily modified
boot_cpu_physical_apicid. As a result, disabled_cpu_apicid kernel
parameter doesn't work well for apicids of APs.
Fixing the wrong handling of boot_cpu_physical_apicid requires some
reviews and tests beyond some platforms and it could take some
time. The fix here is a kind of workaround to focus on the main topic
of this patch.
Signed-off-by: HATAYAMA Daisuke <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: H. Peter Anvin <[email protected]>
---
Documentation/kernel-parameters.txt | 9 +++++++
arch/x86/kernel/apic/apic.c | 49 +++++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+)
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index b9e9bd8..6fdbf8c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -774,6 +774,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
disable= [IPV6]
See Documentation/networking/ipv6.txt.
+ disable_cpu_apicid= [X86,APIC,SMP]
+ Format: <int>
+ The number of initial APIC ID for the
+ corresponding CPU to be disabled at boot,
+ mostly used for the kdump 2nd kernel to
+ disable BSP to wake up multiple CPUs without
+ causing system reset or hang due to sending
+ INIT from AP to BSP.
+
disable_ddw [PPC/PSERIES]
Disable Dynamic DMA Window support. Use this if
to workaround buggy firmware.
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4ec1dd6..e78ab8c 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -75,6 +75,13 @@ unsigned int max_physical_apicid;
physid_mask_t phys_cpu_present_map;
/*
+ * Processor to be disabled specified by kernel parameter
+ * disable_cpu_apicid=<int>, mostly used for the kdump 2nd kernel to
+ * avoid undefined behaviour caused by sending INIT from AP to BSP.
+ */
+unsigned int disabled_cpu_apicid = BAD_APICID;
+
+/*
* Map cpu index to physical APIC ID
*/
DEFINE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_cpu_to_apicid, BAD_APICID);
@@ -2114,6 +2121,39 @@ int generic_processor_info(int apicid, int version)
phys_cpu_present_map);
/*
+ * boot_cpu_physical_apicid is designed to have the apicid
+ * returned by read_apic_id(), i.e, the apicid of the
+ * currently booting-up processor. However, on some platforms,
+ * it is temporarilly modified by the apicid reported as BSP
+ * through MP table. Concretely:
+ *
+ * - arch/x86/kernel/mpparse.c: MP_processor_info()
+ * - arch/x86/mm/amdtopology.c: amd_numa_init()
+ * - arch/x86/platform/visws/visws_quirks.c: MP_processor_info()
+ *
+ * This function is executed with the modified
+ * boot_cpu_physical_apicid. So, disabled_cpu_apicid kernel
+ * parameter doesn't work to disable APs on kdump 2nd kernel.
+ *
+ * Since fixing handling of boot_cpu_physical_apicid requires
+ * another discussion and tests on each platform, we leave it
+ * for now and here we use read_apic_id() directly in this
+ * function, generic_processor_info().
+ */
+ if (disabled_cpu_apicid != BAD_APICID &&
+ disabled_cpu_apicid != read_apic_id() &&
+ disabled_cpu_apicid == apicid) {
+ int thiscpu = num_processors + disabled_cpus;
+
+ pr_warning("ACPI: Disabling requested cpu."
+ " Processor %d/0x%x ignored.\n",
+ thiscpu, apicid);
+
+ disabled_cpus++;
+ return -ENODEV;
+ }
+
+ /*
* If boot cpu has not been detected yet, then only allow upto
* nr_cpu_ids - 1 processors and keep one slot free for boot cpu
*/
@@ -2591,3 +2631,12 @@ static int __init lapic_insert_resource(void)
* that is using request_resource
*/
late_initcall(lapic_insert_resource);
+
+static int __init apic_set_disabled_cpu_apicid(char *arg)
+{
+ if (!arg || !get_option(&arg, &disabled_cpu_apicid))
+ return -EINVAL;
+
+ return 0;
+}
+early_param("disable_cpu_apicid", apic_set_disabled_cpu_apicid);
On Wed, Jan 15, 2014 at 09:54:31AM -0800, H. Peter Anvin wrote:
> On 01/15/2014 09:47 AM, Vivek Goyal wrote:
> > On Wed, Jan 15, 2014 at 09:26:14AM -0800, H. Peter Anvin wrote:
> >> On 01/15/2014 09:05 AM, Vivek Goyal wrote:
> >>>
> >>> I think this is a reasonable approach to solve the issue. Use a command
> >>> line to not bring up specific cpu in second kernel which can create
> >>> problems.
> >>>
> >>> Acked-by: Vivek Goyal <[email protected]>
> >>>
> >>> hpa, I know you are not excited about this approach. If you made up your
> >>> mind that this appoarch is not worth pursuing, please do suggest what
> >>> would you like to see and we can give that a try.
> >>>
> >>> We want to solve this problem as on large memory machines saving dump can
> >>> take lot of time and we want to bring up multiple cpus and speed up
> >>> compression and save on dump time.
> >>>
> >>
> >> I'm not excited about kdump's reliance on the command line, since it
> >> seems to be a neverending source of trouble, simply because the command
> >> line is fundamentally intended as a human interface.
> >
> > So in general, what are the alternatives? Either we figure out that kernel
> > is booting as kdump kernel and do things differently. That seems even
> > worse as what do we want in kdump kernel will change over a period of
> > time.
> >
> > Other thing is that pass more information in bootparams. But that does
> > not seem much different than command line to me.
> >
>
> It is the commingling of semantics that is the problem. Command line
> options are generally imperative, "do this". What you want in the kdump
> situation, as you yourself state above, is get a description of the
> current situation and let the kdump side choose the action to take.
>
> As a transport mechanism the command line suffers from limited size and
> that you have to share it with an arbitrary amount of user-provided
> options that may or may not be essential.
For large amount of info like memory map, I agree that passing on command
line is not a good idea. (/me taks the blame for doing that). That's why
in new patches I want to move to pass new map on bootparams and pass
saved_max_pfn on command line instead. This is a fresh start so we
probably can ignore compatibility with older kernels for this new
interface and set things right.
But for smaller options, command line seems to be good that they don't
consume precious space in bootparams. If we introduce an option today,
we are not sure if kdump will continue to use that option down the line
or not. For example, few years down the line, we might be able to send
INIT IPI to boot cpu too and not need disable_cpu_apicid. Same is the case
with max_cpus vs nr_cpus. We used to use max_cpus=1 and now use nr_cpus=1.
If we put all this informatoin in bootparams, they might soon become
obsolete and keep on sitting there for eternity with no users.
Also by creating a command line, a user can use these knobs as debugging
options and can easily test first kernel's behavior to make sure knob
works well in first kernel before it is tested in second kernel. By making
it part of bootparams, we have no idea whether knob works fine in first
kernel or not.
For above reasons, I am not averse to the idea of commingling.
Thanks
Vivek
On 01/15/2014 10:14 AM, Vivek Goyal wrote:
>
> For large amount of info like memory map, I agree that passing on command
> line is not a good idea. (/me taks the blame for doing that). That's why
> in new patches I want to move to pass new map on bootparams and pass
> saved_max_pfn on command line instead. This is a fresh start so we
> probably can ignore compatibility with older kernels for this new
> interface and set things right.
>
> But for smaller options, command line seems to be good that they don't
> consume precious space in bootparams. If we introduce an option today,
> we are not sure if kdump will continue to use that option down the line
> or not. For example, few years down the line, we might be able to send
> INIT IPI to boot cpu too and not need disable_cpu_apicid. Same is the case
> with max_cpus vs nr_cpus. We used to use max_cpus=1 and now use nr_cpus=1.
> If we put all this informatoin in bootparams, they might soon become
> obsolete and keep on sitting there for eternity with no users.
>
> Also by creating a command line, a user can use these knobs as debugging
> options and can easily test first kernel's behavior to make sure knob
> works well in first kernel before it is tested in second kernel. By making
> it part of bootparams, we have no idea whether knob works fine in first
> kernel or not.
>
> For above reasons, I am not averse to the idea of commingling.
>
bootparams is not a precious resource, and even if it was, you could
create your own (kexec) structure and put it in the boot_info linked
list. bootparams is not a precious resource because is is actually
rather straightforward to extend past 4K should it become necessary; all
we really would need to do there is to include a length field in the
structure. An auxiliary bootparams structure is also a possibility.
-hpa
* tip-bot for HATAYAMA Daisuke <[email protected]> wrote:
> /*
> + * Processor to be disabled specified by kernel parameter
> + * disable_cpu_apicid=<int>, mostly used for the kdump 2nd kernel to
> + * avoid undefined behaviour caused by sending INIT from AP to BSP.
> + */
> +unsigned int disabled_cpu_apicid = BAD_APICID;
Any reason why this isn't static & read_mostly?
> @@ -2114,6 +2121,39 @@ int generic_processor_info(int apicid, int version)
> phys_cpu_present_map);
>
> /*
> + * boot_cpu_physical_apicid is designed to have the apicid
> + * returned by read_apic_id(), i.e, the apicid of the
> + * currently booting-up processor. However, on some platforms,
> + * it is temporarilly modified by the apicid reported as BSP
s/temporarily
> + * through MP table. Concretely:
> + *
> + * - arch/x86/kernel/mpparse.c: MP_processor_info()
> + * - arch/x86/mm/amdtopology.c: amd_numa_init()
> + * - arch/x86/platform/visws/visws_quirks.c: MP_processor_info()
> + *
> + * This function is executed with the modified
> + * boot_cpu_physical_apicid. So, disabled_cpu_apicid kernel
> + * parameter doesn't work to disable APs on kdump 2nd kernel.
> + *
> + * Since fixing handling of boot_cpu_physical_apicid requires
> + * another discussion and tests on each platform, we leave it
> + * for now and here we use read_apic_id() directly in this
> + * function, generic_processor_info().
> + */
> + if (disabled_cpu_apicid != BAD_APICID &&
> + disabled_cpu_apicid != read_apic_id() &&
> + disabled_cpu_apicid == apicid) {
> + int thiscpu = num_processors + disabled_cpus;
> +
> + pr_warning("ACPI: Disabling requested cpu."
> + " Processor %d/0x%x ignored.\n",
> + thiscpu, apicid);
Why does this message say 'ACPI'? Shouldn't it say 'apic' instead?
Thanks,
Ingo
Commit-ID: 5b4d1dbc24bb6fd7179ada0f47be34e27e64decb
Gitweb: http://git.kernel.org/tip/5b4d1dbc24bb6fd7179ada0f47be34e27e64decb
Author: H. Peter Anvin <[email protected]>
AuthorDate: Wed, 15 Jan 2014 13:02:08 -0800
Committer: H. Peter Anvin <[email protected]>
CommitDate: Wed, 15 Jan 2014 13:02:08 -0800
x86, apic: Make disabled_cpu_apicid static read_mostly, fix typos
Make disabled_cpu_apicid static and read_mostly, and fix a couple of
typos.
Reported-by: Ingo Molnar <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: H. Peter Anvin <[email protected]>
Cc: HATAYAMA Daisuke <[email protected]>
---
arch/x86/kernel/apic/apic.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index e78ab8c..7f26c9a 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -79,7 +79,7 @@ physid_mask_t phys_cpu_present_map;
* disable_cpu_apicid=<int>, mostly used for the kdump 2nd kernel to
* avoid undefined behaviour caused by sending INIT from AP to BSP.
*/
-unsigned int disabled_cpu_apicid = BAD_APICID;
+static unsigned int disabled_cpu_apicid __read_mostly = BAD_APICID;
/*
* Map cpu index to physical APIC ID
@@ -2124,7 +2124,7 @@ int generic_processor_info(int apicid, int version)
* boot_cpu_physical_apicid is designed to have the apicid
* returned by read_apic_id(), i.e, the apicid of the
* currently booting-up processor. However, on some platforms,
- * it is temporarilly modified by the apicid reported as BSP
+ * it is temporarily modified by the apicid reported as BSP
* through MP table. Concretely:
*
* - arch/x86/kernel/mpparse.c: MP_processor_info()
@@ -2145,7 +2145,7 @@ int generic_processor_info(int apicid, int version)
disabled_cpu_apicid == apicid) {
int thiscpu = num_processors + disabled_cpus;
- pr_warning("ACPI: Disabling requested cpu."
+ pr_warning("APIC: Disabling requested cpu."
" Processor %d/0x%x ignored.\n",
thiscpu, apicid);
(2014/01/16 6:09), tip-bot for H. Peter Anvin wrote:
> Commit-ID: 5b4d1dbc24bb6fd7179ada0f47be34e27e64decb
> Gitweb: http://git.kernel.org/tip/5b4d1dbc24bb6fd7179ada0f47be34e27e64decb
> Author: H. Peter Anvin <[email protected]>
> AuthorDate: Wed, 15 Jan 2014 13:02:08 -0800
> Committer: H. Peter Anvin <[email protected]>
> CommitDate: Wed, 15 Jan 2014 13:02:08 -0800
>
> x86, apic: Make disabled_cpu_apicid static read_mostly, fix typos
>
> Make disabled_cpu_apicid static and read_mostly, and fix a couple of
> typos.
>
> Reported-by: Ingo Molnar <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> Signed-off-by: H. Peter Anvin <[email protected]>
> Cc: HATAYAMA Daisuke <[email protected]>
> ---
> arch/x86/kernel/apic/apic.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index e78ab8c..7f26c9a 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -79,7 +79,7 @@ physid_mask_t phys_cpu_present_map;
> * disable_cpu_apicid=<int>, mostly used for the kdump 2nd kernel to
> * avoid undefined behaviour caused by sending INIT from AP to BSP.
> */
> -unsigned int disabled_cpu_apicid = BAD_APICID;
> +static unsigned int disabled_cpu_apicid __read_mostly = BAD_APICID;
>
> /*
> * Map cpu index to physical APIC ID
> @@ -2124,7 +2124,7 @@ int generic_processor_info(int apicid, int version)
> * boot_cpu_physical_apicid is designed to have the apicid
> * returned by read_apic_id(), i.e, the apicid of the
> * currently booting-up processor. However, on some platforms,
> - * it is temporarilly modified by the apicid reported as BSP
> + * it is temporarily modified by the apicid reported as BSP
> * through MP table. Concretely:
> *
> * - arch/x86/kernel/mpparse.c: MP_processor_info()
> @@ -2145,7 +2145,7 @@ int generic_processor_info(int apicid, int version)
> disabled_cpu_apicid == apicid) {
> int thiscpu = num_processors + disabled_cpus;
>
> - pr_warning("ACPI: Disabling requested cpu."
> + pr_warning("APIC: Disabling requested cpu."
> " Processor %d/0x%x ignored.\n",
> thiscpu, apicid);
This is not typo in my intention.
generic_processor_info() has two more cases where it ignores cpus.
In either cases, printed messages are tagged with "ACPI" because this
function is called when parsing ACPI MADT table in acpi_boot_init();
this function is also being used to parse other kind of tables but
the "ACPI" tag would mean that the function was first for ACPI only.
int generic_processor_info(int apicid, int version)
{
int cpu, max = nr_cpu_ids;
bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
phys_cpu_present_map);
/*
* If boot cpu has not been detected yet, then only allow upto
* nr_cpu_ids - 1 processors and keep one slot free for boot cpu
*/
if (!boot_cpu_detected && num_processors >= nr_cpu_ids - 1 &&
apicid != boot_cpu_physical_apicid) {
int thiscpu = max + disabled_cpus - 1;
pr_warning(
"ACPI: NR_CPUS/possible_cpus limit of %i almost"
" reached. Keeping one slot for boot cpu."
" Processor %d/0x%x ignored.\n", max, thiscpu, apicid);
disabled_cpus++;
return -ENODEV;
}
if (num_processors >= nr_cpu_ids) {
int thiscpu = max + disabled_cpus;
pr_warning(
"ACPI: NR_CPUS/possible_cpus limit of %i reached."
" Processor %d/0x%x ignored.\n", max, thiscpu, apicid);
disabled_cpus++;
return -EINVAL;
}
--
Thanks.
HATAYAMA, Daisuke
On 01/15/2014 08:44 PM, HATAYAMA Daisuke wrote:
>
> This is not typo in my intention.
>
> generic_processor_info() has two more cases where it ignores cpus.
> In either cases, printed messages are tagged with "ACPI" because this
> function is called when parsing ACPI MADT table in acpi_boot_init();
> this function is also being used to parse other kind of tables but
> the "ACPI" tag would mean that the function was first for ACPI only.
>
But it has nothing to do with ACPI -- it is an APIC ID from the command
line -- so that would be actively misleading.
-hpa
(2014/01/16 14:53), H. Peter Anvin wrote:
> On 01/15/2014 08:44 PM, HATAYAMA Daisuke wrote:
>>
>> This is not typo in my intention.
>>
>> generic_processor_info() has two more cases where it ignores cpus.
>> In either cases, printed messages are tagged with "ACPI" because this
>> function is called when parsing ACPI MADT table in acpi_boot_init();
>> this function is also being used to parse other kind of tables but
>> the "ACPI" tag would mean that the function was first for ACPI only.
>>
>
> But it has nothing to do with ACPI -- it is an APIC ID from the command
> line -- so that would be actively misleading.
>
> -hpa
>
I never disagree to the fix itself. I wanted to explain why I wrote so.
I thought it was better to unify tags in the same function because they
should bleong to the same component, here I mean ACPI, but it's better
to avoid the confusion, which is bigger impact.
--
Thanks.
HATAYAMA, Daisuke
On 01/15/2014 10:20 PM, HATAYAMA Daisuke wrote:
> (2014/01/16 14:53), H. Peter Anvin wrote:
>> On 01/15/2014 08:44 PM, HATAYAMA Daisuke wrote:
>>>
>>> This is not typo in my intention.
>>>
>>> generic_processor_info() has two more cases where it ignores cpus.
>>> In either cases, printed messages are tagged with "ACPI" because this
>>> function is called when parsing ACPI MADT table in acpi_boot_init();
>>> this function is also being used to parse other kind of tables but
>>> the "ACPI" tag would mean that the function was first for ACPI only.
>>>
>>
>> But it has nothing to do with ACPI -- it is an APIC ID from the command
>> line -- so that would be actively misleading.
>>
>> -hpa
>>
>
> I never disagree to the fix itself. I wanted to explain why I wrote so.
>
> I thought it was better to unify tags in the same function because they
> should bleong to the same component, here I mean ACPI, but it's better
> to avoid the confusion, which is bigger impact.
>
Indeed. All good now.
-hpa
On Wed, 15 Jan 2014 13:14:26 -0500
Vivek Goyal <[email protected]> wrote:
> On Wed, Jan 15, 2014 at 09:54:31AM -0800, H. Peter Anvin wrote:
> > On 01/15/2014 09:47 AM, Vivek Goyal wrote:
> > > On Wed, Jan 15, 2014 at 09:26:14AM -0800, H. Peter Anvin wrote:
> > >> On 01/15/2014 09:05 AM, Vivek Goyal wrote:
> > >>>
> > >>> I think this is a reasonable approach to solve the issue. Use a command
> > >>> line to not bring up specific cpu in second kernel which can create
> > >>> problems.
> > >>>
> > >>> Acked-by: Vivek Goyal <[email protected]>
> > >>>
> > >>> hpa, I know you are not excited about this approach. If you made up your
> > >>> mind that this appoarch is not worth pursuing, please do suggest what
> > >>> would you like to see and we can give that a try.
> > >>>
> > >>> We want to solve this problem as on large memory machines saving dump can
> > >>> take lot of time and we want to bring up multiple cpus and speed up
> > >>> compression and save on dump time.
> > >>>
> > >>
> > >> I'm not excited about kdump's reliance on the command line, since it
> > >> seems to be a neverending source of trouble, simply because the command
> > >> line is fundamentally intended as a human interface.
> > >
> > > So in general, what are the alternatives? Either we figure out that kernel
> > > is booting as kdump kernel and do things differently. That seems even
> > > worse as what do we want in kdump kernel will change over a period of
> > > time.
> > >
> > > Other thing is that pass more information in bootparams. But that does
> > > not seem much different than command line to me.
> > >
> >
> > It is the commingling of semantics that is the problem. Command line
> > options are generally imperative, "do this". What you want in the kdump
> > situation, as you yourself state above, is get a description of the
> > current situation and let the kdump side choose the action to take.
> >
> > As a transport mechanism the command line suffers from limited size and
> > that you have to share it with an arbitrary amount of user-provided
> > options that may or may not be essential.
>
> For large amount of info like memory map, I agree that passing on command
> line is not a good idea. (/me taks the blame for doing that). That's why
> in new patches I want to move to pass new map on bootparams and pass
> saved_max_pfn on command line instead. This is a fresh start so we
> probably can ignore compatibility with older kernels for this new
> interface and set things right.
>
> But for smaller options, command line seems to be good that they don't
> consume precious space in bootparams. If we introduce an option today,
> we are not sure if kdump will continue to use that option down the line
> or not. For example, few years down the line, we might be able to send
> INIT IPI to boot cpu too and not need disable_cpu_apicid.
Yes, that was my thinking. We really only need all this, because
current BIOS implementations do not offer a way to override the default
initialization routine on a BSP. In fact, I disassembled the INIT
vector of a few BIOSes to see if the old 286 way of leaving protected
mode (via a CMOS location) is still supported and might be used at
least on some hardware. It is not, but that doesn't mean future BIOSes
may not re-implement something like that.
Sorry for coming late to the party...
Petr Tesarik