[PATCH] ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus=

[PATCH] ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus=

[Thomas Renninger]

Patch is against latest Linus master branch and is expected to be
safe bug fix.

You get:
ACPI: HARDWARE addr space,NOT supported yet
for each ACPI defined CPU which status is active, but exceeds
maxcpus= count.

As these "not booted" CPUs do not run an idle routine
and echo X >/proc/acpi/processor/*/throttling did not work
I couldn't find a way to really access not onlined/booted
machines. Still this should get fixed and
/proc/acpi/processor/X dirs of cores exceeding maxcpus
should not show up.

I wonder whether this could get cleaned up by truncating possible cpu mask
and nr_cpu_ids to setup_max_cpus early some day
(and not exporting setup_max_cpus anymore then).
But this needs touching of a lot other places...

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: travis@sgi.com
CC: linux-acpi@vger.kernel.org
CC: lenb@kernel.org
---
 drivers/acpi/processor_driver.c |    5 +++++
 init/main.c                     |    4 +++-
 2 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 5675d97..7615bc2 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -581,6 +581,11 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
 		return 0;
 	}
 
+#ifdef CONFIG_SMP
+	if (pr->id >= setup_max_cpus && pr->id != 0)
+		return 0;
+#endif
+
 	BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0));
 
 	/*
diff --git a/init/main.c b/init/main.c
index 22881b5..fc104db 100644
--- a/init/main.c
+++ b/init/main.c
@@ -125,7 +125,9 @@ static char *ramdisk_execute_command;
 
 #ifdef CONFIG_SMP
 /* Setup configured maximum number of CPUs to activate */
-unsigned int __initdata setup_max_cpus = NR_CPUS;
+unsigned int setup_max_cpus = NR_CPUS;
+EXPORT_SYMBOL(setup_max_cpus);
+
 
 /*
  * Setup routine for controlling SMP activation
-- 
1.6.3

Re: [PATCH] ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus=

[Len Brown]

On Wed, 26 May 2010, Thomas Renninger wrote:

> Patch is against latest Linus master branch and is expected to be
> safe bug fix.
> 
> You get:
> ACPI: HARDWARE addr space,NOT supported yet
> for each ACPI defined CPU which status is active, but exceeds
> maxcpus= count.
> 
> As these "not booted" CPUs do not run an idle routine
> and echo X >/proc/acpi/processor/*/throttling did not work
> I couldn't find a way to really access not onlined/booted
> machines. Still this should get fixed and
> /proc/acpi/processor/X dirs of cores exceeding maxcpus
> should not show up.

Can we delete the /proc/acpi/processor/ dirs instead of maintaining them?

thanks,
-Len Brown, Intel Open Source Technology Center

Re: [PATCH] ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus=

[Thomas Renninger]

On Friday 28 May 2010 08:46:27 pm Len Brown wrote:
> On Wed, 26 May 2010, Thomas Renninger wrote:
> > Patch is against latest Linus master branch and is expected to be
> > safe bug fix.
> >
> > You get:
> > ACPI: HARDWARE addr space,NOT supported yet
> > for each ACPI defined CPU which status is active, but exceeds
> > maxcpus= count.
> >
> > As these "not booted" CPUs do not run an idle routine
> > and echo X >/proc/acpi/processor/*/throttling did not work
> > I couldn't find a way to really access not onlined/booted
> > machines. Still this should get fixed and
> > /proc/acpi/processor/X dirs of cores exceeding maxcpus
> > should not show up.
>
> Can we delete the /proc/acpi/processor/ dirs instead of maintaining them?

This is not about "not creating proc files", but about setting up 
throttling/c-states  for not booted CPUs.
The message:
ACPI: HARDWARE addr space,NOT supported yet
comes from throttling init code. It goes an undefined path because

struct cpuinfo_x86 *c = &cpu_data(cpu);
is uninitialized (all is zero...):
if ((c->x86_vendor != X86_VENDOR_INTEL) ||
                !cpu_has(c, X86_FEATURE_ACPI)) {
                printk(KERN_ERR PREFIX
                        "HARDWARE addr space,NOT supported yet\n");

Similar to what happens (happened?) on acpi processor (real) hotplug.

The proc files should vanish in a separate patch, but I am not sure what
could break.
I saw powertop still reading battery info from /proc, I thought hal is also 
still accessing it, but I cannot say for sure.

      Thomas

Re: [PATCH] ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus=

[ykzhao]

On Sat, 2010-05-29 at 03:25 +0800, Thomas Renninger wrote:
> On Friday 28 May 2010 08:46:27 pm Len Brown wrote:
> > On Wed, 26 May 2010, Thomas Renninger wrote:
> > > Patch is against latest Linus master branch and is expected to be
> > > safe bug fix.
> > >
> > > You get:
> > > ACPI: HARDWARE addr space,NOT supported yet
> > > for each ACPI defined CPU which status is active, but exceeds
> > > maxcpus= count.
> > >
> > > As these "not booted" CPUs do not run an idle routine
> > > and echo X >/proc/acpi/processor/*/throttling did not work
> > > I couldn't find a way to really access not onlined/booted
> > > machines. Still this should get fixed and
> > > /proc/acpi/processor/X dirs of cores exceeding maxcpus
> > > should not show up.
> >
> > Can we delete the /proc/acpi/processor/ dirs instead of maintaining them?
> 
> This is not about "not creating proc files", but about setting up 
> throttling/c-states  for not booted CPUs.
> The message:
> ACPI: HARDWARE addr space,NOT supported yet
> comes from throttling init code. It goes an undefined path because
> 
> struct cpuinfo_x86 *c = &cpu_data(cpu);
> is uninitialized (all is zero...):
> if ((c->x86_vendor != X86_VENDOR_INTEL) ||
>                 !cpu_has(c, X86_FEATURE_ACPI)) {
>                 printk(KERN_ERR PREFIX
>                         "HARDWARE addr space,NOT supported yet\n");

Agree with your analysis.

Can we add "online check" in the above throtting patch or simply use the
cpuinfo of the first boot CPU to fix the corresponding warning message?
Maybe we will change the T-state for the hot-onlined cpu.

thanks.

> 
> Similar to what happens (happened?) on acpi processor (real) hotplug.
> 
> The proc files should vanish in a separate patch, but I am not sure what
> could break.
> I saw powertop still reading battery info from /proc, I thought hal is also 
> still accessing it, but I cannot say for sure.
> 
>       Thomas
> --
> 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

Re: [PATCH] ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus=

[Thomas Renninger]

On Wednesday 02 June 2010 03:22:46 am ykzhao wrote:
> On Sat, 2010-05-29 at 03:25 +0800, Thomas Renninger wrote:
...
> > This is not about "not creating proc files", but about setting up
> > throttling/c-states  for not booted CPUs.
> > The message:
> > ACPI: HARDWARE addr space,NOT supported yet
> > comes from throttling init code. It goes an undefined path because
> >
> > struct cpuinfo_x86 *c = &cpu_data(cpu);
> > is uninitialized (all is zero...):
> > if ((c->x86_vendor != X86_VENDOR_INTEL) ||
> >                 !cpu_has(c, X86_FEATURE_ACPI)) {
> >                 printk(KERN_ERR PREFIX
> >                         "HARDWARE addr space,NOT supported yet\n");
>
> Agree with your analysis.
>
> Can we add "online check" in the above throtting patch
Not really. You get corner cases you need to check for:
  CPUs get offlined manually -> processor driver gets loaded ->
  CPU gets onlined again -> Throttling/C-states wil be uninitialized.

> or simply use the 
> cpuinfo of the first boot CPU to fix the corresponding warning message?
This is an ugly hack (I also thought about it to be honest). Then the whole or 
quite some variables of the per_cpu cpuinfo_x86 structure could be made 
global and need not to be per_cpu based. This also indicates that something
in the processor driver is wrong by design.
> Maybe we will change the T-state for the hot-onlined cpu.
Not sure I understand this sentence:
If the CPU is beyond maxcpus=X you must not be able to change T-states
for this CPU.

Hm, the hotplug cleanup patch I sent recently would take care of above and 
make this tiny sanity check unnecessary (I sent that just for Len to get a 
picture what's missing in procoessor driver code for real CPU hotplug):
http://permalink.gmane.org/gmane.linux.power-management.general/19491
There still were issues with this patch (not sure when I try on this again, it 
has low prio for me currently), thus I'd appreciate if this safe sanity check 
could just go to Linus.

Thanks,

      Thomas

Re: [PATCH] ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus=

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center