acpi_idle and max_cpus

Hi all,

I have a dual core Intel T9500.

I boot the cpu with the acpi_idle driver and intel_idle enabled in the config.

The kernel is booted with maxcpus=1.

After the system has boot, I put cpu1 online via sysfs.

But I don't see any 'cpuidle' directory in the cpu's sysfs entry:

/sys/devices/system/cpu/cpu1/cpuidle (?)

When I look at the code I see the notifier is present for hotplug in processor_driver.c and the cpuidle intel init routine should be called there.

I am wondering is it a bug or an expected behavior ?

Quoted 12 lines · Daniel Lezcano

Hi all,

I have a dual core Intel T9500.

I boot the cpu with the acpi_idle driver and intel_idle enabled in the config.

The kernel is booted with maxcpus=1.

After the system has boot, I put cpu1 online via sysfs.

Quoted 8 lines

But I don't see any 'cpuidle' directory in the cpu's sysfs entry:

/sys/devices/system/cpu/cpu1/cpuidle (?)

When I look at the code I see the notifier is present for hotplug in processor_driver.c and the cpuidle intel init routine should be called there.

Quoted 1 line

I am wondering is it a bug or an expected behavior ?

Any thoughts on that ?

Thanks
-- Daniel

Hi Daniel,

Quoted 13 lines · Daniel Lezcano

Quoted 12 lines · Daniel Lezcano

Hi all,

I have a dual core Intel T9500.

I boot the cpu with the acpi_idle driver and intel_idle enabled in the config.

The kernel is booted with maxcpus=1.

After the system has boot, I put cpu1 online via sysfs.

Quoted 8 lines

Quoted 8 lines

But I don't see any 'cpuidle' directory in the cpu's sysfs entry:

/sys/devices/system/cpu/cpu1/cpuidle (?)

When I look at the code I see the notifier is present for hotplug in processor_driver.c and the cpuidle intel init routine should be called there.

Yes, we have a hotplug notifier.
Commit 99b72508 by Thomas Renninger fixed this issue.

Please let me know which kernel version you are running and what is idle driver registered ?

Quoted 8 lines

Quoted 1 line

I am wondering is it a bug or an expected behavior ?

Any thoughts on that ?

Thanks
-- Daniel

Cheers
Deepthi

Quoted 27 lines · Deepthi Dharwar

Hi Daniel,

Quoted 13 lines · Daniel Lezcano

Quoted 12 lines · Daniel Lezcano

Hi all,

I have a dual core Intel T9500.

I boot the cpu with the acpi_idle driver and intel_idle enabled in the config.

The kernel is booted with maxcpus=1.

After the system has boot, I put cpu1 online via sysfs.

Quoted 8 lines

Quoted 8 lines

But I don't see any 'cpuidle' directory in the cpu's sysfs entry:

/sys/devices/system/cpu/cpu1/cpuidle (?)

When I look at the code I see the notifier is present for hotplug in processor_driver.c and the cpuidle intel init routine should be called there.

Quoted 5 lines

Yes, we have a hotplug notifier.
Commit 99b72508 by Thomas Renninger fixed this issue.

Please let me know which kernel version you are running and what is idle driver registered ?

The kernel version is 3.5.0-rc1
The registered driver is acpi_idle (with intel_idle if I am not wrong).

Thomas's patch is in this version.

Maybe I am wrong but I think the patch is not correct because:

static int __cpuinit acpi_processor_add(struct acpi_device *device) {

...

#ifdef CONFIG_SMP
	if (pr->id >= setup_max_cpus && pr->id != 0)
		return 0;

#endif

...

	per_cpu(processors, pr->id) = pr;

...
}

With max_cpus=1 we exit before setting up 'pr'.

So the condition in:

static int acpi_cpu_soft_notify(...)
{

	unsigned int cpu = (unsigned long)hcpu;
	struct acpi_processor *pr = per_cpu(processors, cpu);

	if (action == CPU_ONLINE && pr) {

...
}

Is always false because pr == NULL

I did the change but I don't still see the 'cpuidle' directory appearing, I suspect also pr->flags.need_hotplug_init is not correctly initialized but I did not investigate more.

Quoted 75 lines · Daniel Lezcano

Quoted 27 lines · Deepthi Dharwar

Hi Daniel,

Quoted 13 lines · Daniel Lezcano

Quoted 12 lines · Daniel Lezcano

Hi all,

I have a dual core Intel T9500.

I boot the cpu with the acpi_idle driver and intel_idle enabled in the config.

The kernel is booted with maxcpus=1.

After the system has boot, I put cpu1 online via sysfs.

Quoted 8 lines

Quoted 8 lines

But I don't see any 'cpuidle' directory in the cpu's sysfs entry:

/sys/devices/system/cpu/cpu1/cpuidle (?)

When I look at the code I see the notifier is present for hotplug in processor_driver.c and the cpuidle intel init routine should be called there.

Quoted 5 lines

Yes, we have a hotplug notifier.
Commit 99b72508 by Thomas Renninger fixed this issue.

Please let me know which kernel version you are running and what is idle driver registered ?

The kernel version is 3.5.0-rc1
The registered driver is acpi_idle (with intel_idle if I am not wrong).

Thomas's patch is in this version.

Maybe I am wrong but I think the patch is not correct because:

static int __cpuinit acpi_processor_add(struct acpi_device *device) {

...

#ifdef CONFIG_SMP
	if (pr->id >= setup_max_cpus && pr->id != 0)
		return 0;

#endif

...

	per_cpu(processors, pr->id) = pr;

...
}

With max_cpus=1 we exit before setting up 'pr'.

So the condition in:

static int acpi_cpu_soft_notify(...)
{

	unsigned int cpu = (unsigned long)hcpu;
	struct acpi_processor *pr = per_cpu(processors, cpu);

	if (action == CPU_ONLINE && pr) {

...
}

Is always false because pr == NULL

Quoted 4 lines

I did the change but I don't still see the 'cpuidle' directory appearing, I suspect also pr->flags.need_hotplug_init is not correctly initialized but I did not investigate more.

Well looks like variable maxcpus holds the key here.

When I am booting the system, say with 2 out of 4 available cpus, set via maxcpus variable with intel_idle or acpi_idle and onlining the other 2 cpus later via sysfs, I dont see cpufreq or cpuidle dir in the sysfs path.

# cd /sys/devices/system/cpu/cpu2
xxx:/sys/devices/system/cpu/cpu2# ls
	crash_notes  node0  online
xxx:/sys/devices/system/cpu/cpu2# echo 1 > online
xxx:/sys/devices/system/cpu/cpu2# ls
cache  crash_notes  node0  online  thermal_throttle  topology

So looks like its designed that way for now.

So if maxcpus=X, X<Y where Y is no of available cpus. Enabling the Y-X cpus later after the boot via sysfs is not enabling cpuidle and cpufreq .

The question is, do we want to modify this behavior ?

Quoted 77 lines · Deepthi Dharwar

Quoted 75 lines · Daniel Lezcano

Quoted 27 lines · Deepthi Dharwar

Hi Daniel,

Quoted 13 lines · Daniel Lezcano

Quoted 12 lines · Daniel Lezcano

Hi all,

I have a dual core Intel T9500.

I boot the cpu with the acpi_idle driver and intel_idle enabled in the config.

The kernel is booted with maxcpus=1.

After the system has boot, I put cpu1 online via sysfs.

Quoted 8 lines

Quoted 8 lines

But I don't see any 'cpuidle' directory in the cpu's sysfs entry:

/sys/devices/system/cpu/cpu1/cpuidle (?)

When I look at the code I see the notifier is present for hotplug in processor_driver.c and the cpuidle intel init routine should be called there.

Quoted 5 lines

Yes, we have a hotplug notifier.
Commit 99b72508 by Thomas Renninger fixed this issue.

Please let me know which kernel version you are running and what is idle driver registered ?

The kernel version is 3.5.0-rc1
The registered driver is acpi_idle (with intel_idle if I am not wrong).

Thomas's patch is in this version.

Maybe I am wrong but I think the patch is not correct because:

static int __cpuinit acpi_processor_add(struct acpi_device *device) {

...

#ifdef CONFIG_SMP
	if (pr->id >= setup_max_cpus && pr->id != 0)
		return 0;

#endif

...

	per_cpu(processors, pr->id) = pr;

...
}

With max_cpus=1 we exit before setting up 'pr'.

So the condition in:

static int acpi_cpu_soft_notify(...)
{

	unsigned int cpu = (unsigned long)hcpu;
	struct acpi_processor *pr = per_cpu(processors, cpu);

	if (action == CPU_ONLINE && pr) {

...
}

Is always false because pr == NULL

Quoted 8 lines

Quoted 4 lines

I did the change but I don't still see the 'cpuidle' directory appearing, I suspect also pr->flags.need_hotplug_init is not correctly initialized but I did not investigate more.

Well looks like variable maxcpus holds the key here.

Whose equivalent is setup_max_cpus inside the kernel, as Daniel mentioned.

Quoted 5 lines

When I am booting the system, say with 2 out of 4 available cpus, set via maxcpus variable with intel_idle or acpi_idle and onlining the other 2 cpus later via sysfs, I dont see cpufreq or cpuidle dir in the sysfs path.

Quoted 8 lines

# cd /sys/devices/system/cpu/cpu2
xxx:/sys/devices/system/cpu/cpu2# ls
	crash_notes  node0  online
xxx:/sys/devices/system/cpu/cpu2# echo 1 > online
xxx:/sys/devices/system/cpu/cpu2# ls
cache  crash_notes  node0  online  thermal_throttle  topology

So looks like its designed that way for now.

I don't think so. Looks more like a bug than a design ;-)

Quoted 4 lines

So if maxcpus=X, X<Y where Y is no of available cpus. Enabling the Y-X cpus later after the boot via sysfs is not enabling cpuidle and cpufreq .

Quoted 2 lines

The question is, do we want to modify this behavior ?

Yes we do, and that's Daniel's pain point as well. I don't think there is any good reason why the cpuidle directory must _not_ be exported for cpus that are onlined later.

Quoted 116 lines · Srivatsa S. Bhat

Quoted 77 lines · Deepthi Dharwar

Quoted 75 lines · Daniel Lezcano

Quoted 27 lines · Deepthi Dharwar

Hi Daniel,

Quoted 13 lines · Daniel Lezcano

Quoted 12 lines · Daniel Lezcano

Hi all,

I have a dual core Intel T9500.

I boot the cpu with the acpi_idle driver and intel_idle enabled in the config.

The kernel is booted with maxcpus=1.

After the system has boot, I put cpu1 online via sysfs.

Quoted 8 lines

Quoted 8 lines

But I don't see any 'cpuidle' directory in the cpu's sysfs entry:

/sys/devices/system/cpu/cpu1/cpuidle (?)

When I look at the code I see the notifier is present for hotplug in processor_driver.c and the cpuidle intel init routine should be called there.

Quoted 5 lines

Yes, we have a hotplug notifier.
Commit 99b72508 by Thomas Renninger fixed this issue.

Please let me know which kernel version you are running and what is idle driver registered ?

The kernel version is 3.5.0-rc1
The registered driver is acpi_idle (with intel_idle if I am not wrong).

Thomas's patch is in this version.

Maybe I am wrong but I think the patch is not correct because:

static int __cpuinit acpi_processor_add(struct acpi_device *device) {

...

#ifdef CONFIG_SMP
	if (pr->id >= setup_max_cpus && pr->id != 0)
		return 0;

#endif

...

	per_cpu(processors, pr->id) = pr;

...
}

With max_cpus=1 we exit before setting up 'pr'.

So the condition in:

static int acpi_cpu_soft_notify(...)
{

	unsigned int cpu = (unsigned long)hcpu;
	struct acpi_processor *pr = per_cpu(processors, cpu);

	if (action == CPU_ONLINE && pr) {

...
}

Is always false because pr == NULL

Quoted 8 lines

Quoted 4 lines

I did the change but I don't still see the 'cpuidle' directory appearing, I suspect also pr->flags.need_hotplug_init is not correctly initialized but I did not investigate more.

Well looks like variable maxcpus holds the key here.

Whose equivalent is setup_max_cpus inside the kernel, as Daniel mentioned.

Quoted 5 lines

When I am booting the system, say with 2 out of 4 available cpus, set via maxcpus variable with intel_idle or acpi_idle and onlining the other 2 cpus later via sysfs, I dont see cpufreq or cpuidle dir in the sysfs path.

Quoted 8 lines

# cd /sys/devices/system/cpu/cpu2
xxx:/sys/devices/system/cpu/cpu2# ls
	crash_notes  node0  online
xxx:/sys/devices/system/cpu/cpu2# echo 1 > online
xxx:/sys/devices/system/cpu/cpu2# ls
cache  crash_notes  node0  online  thermal_throttle  topology

So looks like its designed that way for now.

I don't think so. Looks more like a bug than a design ;-)

Quoted 6 lines

So if maxcpus=X, X<Y where Y is no of available cpus. Enabling the Y-X cpus later after the boot via sysfs is not enabling cpuidle and cpufreq .

The question is, do we want to modify this behavior ?

Quoted 3 lines

Yes we do, and that's Daniel's pain point as well. I don't think there is any good reason why the cpuidle directory must _not_ be exported for cpus that are onlined later.

Yes and if I refer to the code, it is supposed to do that.

I added Thomas in Cc.

Daniel Lezcano noticed that after booting with maxcpus=X, if we online the remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then for the newly onlined cpus, the cpuidle directory is not found under /sys/devices/system/cpu/cpuY.

Partly, the reason for this is that acpi restricts the initialization to cpus within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is used to restrict the number of cpus brought up during boot. That doesn't mean that we should hard restrict the bring up of the remaining cpus later on.

So
teach acpi to play nice with maxcpus= parameter, and perform the initialization for all present cpus, but defer their startup to the point when they are actually onlined later.

But that alone won't suffice as a fix, because there is one more thing that the present but !online cpus lack - the need_hotplug_init flag. The need_hotplug_init flag is set only for physically hotplugged cpus and not for cpus which were already present but not brought online during boot (due to the maxcpus= parameter). For cpus with this flag set, during the online operation, acpi_cpu_soft_notify() takes care of the registration with cpuidle. So, in order to allow the present but !online cpus to be onlined later with full features (by making use of acpi_cpu_soft_notify()), set their need_hotplug_init flag.

Reported-by Daniel Lezcano <[email protected]>

Signed-off-by Srivatsa S. Bhat <[email protected]>

---

Daniel, this patch works for me. Does it work for you as well?

 drivers/acpi/processor_driver.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

drivers/acpi/processor_driver.c +11 -5

index 0734086..f29d30f 100644
@@ -551,11 +551,6 @@ 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));
 
 	/*

@@ -580,6 +575,17 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
 		goto err_clear_processor;
 	}
 
+#ifdef CONFIG_SMP
+	if (pr->id >= setup_max_cpus && pr->id != 0) {
+		/*
+		 * Don't start cpus beyond maxcpus now. But allow them to
+		 * to be brought online later.
+		 */
+		pr->flags.need_hotplug_init = 1;
+		return 0;
+	}
+#endif
+
 	/*
 	 * Do not start hotplugged CPUs now, but when they
 	 * are onlined the first time

Quoted 11 lines · Srivatsa S. Bhat

Daniel Lezcano noticed that after booting with maxcpus=X, if we online the remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then for the newly onlined cpus, the cpuidle directory is not found under /sys/devices/system/cpu/cpuY.

Partly, the reason for this is that acpi restricts the initialization to cpus within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is used to restrict the number of cpus brought up during boot. That doesn't mean that we should hard restrict the bring up of the remaining cpus later on.

Sorry, but IMO it exaclty does mean that (adding more general lists for further comments).

If you can online more cores than maxcpus= via sysfs, this sounds like a bug. Not the other way around.

Compare with Documentation/kernel-parameters.txt:
maxcpus= [SMP] Maximum number of processors that an SMP kernel
should make use of. maxcpus=n : n >= 0 limits the kernel to using 'n' processors. n=0 is a special case, it is equivalent to "nosmp", which also disables the IO APIC.

Chances that you run into more problems are high.

It would help if it is explained why at all this would be needed.

If there really is a valid use-case, possibly a bootcpus= param could get defined or above documentation is adjusted. But this would need thorough double checking, because I expect maxcpus=X was never intended to bring up more than X cores later via sysfs and I expect there are more places where things have to get ajusted.

Quoted 63 lines

So
teach acpi to play nice with maxcpus= parameter, and perform the initialization for all present cpus, but defer their startup to the point when they are actually onlined later.

But that alone won't suffice as a fix, because there is one more thing that the present but !online cpus lack - the need_hotplug_init flag. The need_hotplug_init flag is set only for physically hotplugged cpus and not for cpus which were already present but not brought online during boot (due to the maxcpus= parameter). For cpus with this flag set, during the online operation, acpi_cpu_soft_notify() takes care of the registration with cpuidle. So, in order to allow the present but !online cpus to be onlined later with full features (by making use of acpi_cpu_soft_notify()), set their need_hotplug_init flag.

Reported-by Daniel Lezcano <[email protected]>

Signed-off-by Srivatsa S. Bhat <[email protected]>

---

Daniel, this patch works for me. Does it work for you as well?

 drivers/acpi/processor_driver.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

drivers/acpi/processor_driver.c +11 -5

index 0734086..f29d30f 100644
@@ -551,11 +551,6 @@ 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));
 
 	/*

@@ -580,6 +575,17 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
 		goto err_clear_processor;
 	}
 
+#ifdef CONFIG_SMP
+	if (pr->id >= setup_max_cpus && pr->id != 0) {
+		/*
+		 * Don't start cpus beyond maxcpus now. But allow them to
+		 * to be brought online later.
+		 */
+		pr->flags.need_hotplug_init = 1;
+		return 0;
+	}
+#endif
+
 	/*
 	 * Do not start hotplugged CPUs now, but when they
 	 * are onlined the first time

Quoted 26 lines · Thomas Renninger

Quoted 11 lines · Srivatsa S. Bhat

Daniel Lezcano noticed that after booting with maxcpus=X, if we online the remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then for the newly onlined cpus, the cpuidle directory is not found under /sys/devices/system/cpu/cpuY.

Partly, the reason for this is that acpi restricts the initialization to cpus within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is used to restrict the number of cpus brought up during boot. That doesn't mean that we should hard restrict the bring up of the remaining cpus later on.

Sorry, but IMO it exaclty does mean that (adding more general lists for further comments).

If you can online more cores than maxcpus= via sysfs, this sounds like a bug. Not the other way around.

Compare with Documentation/kernel-parameters.txt:
maxcpus= [SMP] Maximum number of processors that an SMP kernel
should make use of. maxcpus=n : n >= 0 limits the kernel to using 'n' processors. n=0 is a special case, it is equivalent to "nosmp", which also disables the IO APIC.

Quoted 1 line

Chances that you run into more problems are high.

Right, I agree on that. So, IMHO, maxcpus=X doesn't mean that the kernel must and should forbid any new cpus from coming online, but in the interest of avoiding problems/complications in some obscure paths, I guess it makes sense to avoid onlining new cpus beyond maxcpus.

In any case, I was just trying to see why the simple removal of the setup_max_cpus check in acpi_processor_add() wasn't enough to expose the cpuidle directories under the new cpus.. and while debugging that, I came up with this patch. I don't mind if this doesn't get picked up.

Right, the usecase of why somebody would like to online new cpus beyond maxcpus doesn't look all that solid anyway. So I am OK with leaving the code as it is now.

Quoted 28 lines · Srivatsa S. Bhat

Quoted 26 lines · Thomas Renninger

Quoted 11 lines · Srivatsa S. Bhat

Daniel Lezcano noticed that after booting with maxcpus=X, if we online the remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then for the newly onlined cpus, the cpuidle directory is not found under /sys/devices/system/cpu/cpuY.

Partly, the reason for this is that acpi restricts the initialization to cpus within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is used to restrict the number of cpus brought up during boot. That doesn't mean that we should hard restrict the bring up of the remaining cpus later on.

Sorry, but IMO it exaclty does mean that (adding more general lists for further comments).

If you can online more cores than maxcpus= via sysfs, this sounds like a bug. Not the other way around.

Compare with Documentation/kernel-parameters.txt:
maxcpus= [SMP] Maximum number of processors that an SMP kernel
should make use of. maxcpus=n : n >= 0 limits the kernel to using 'n' processors. n=0 is a special case, it is equivalent to "nosmp", which also disables the IO APIC.

Quoted 7 lines

Quoted 1 line

Chances that you run into more problems are high.

Right, I agree on that. So, IMHO, maxcpus=X doesn't mean that the kernel must and should forbid any new cpus from coming online, but in the interest of avoiding problems/complications in some obscure paths, I guess it makes sense to avoid onlining new cpus beyond maxcpus.

Yep, for such reasons:
- That nobody realizes this to be useful and makes use of it in a productive environment
- If I see maxcpus=X in a bugreport's dmesg command line,
I want to be sure that's true.
- To enforce that things work as documented

Wow, after looking a bit into this I found (Documentation/cpu-hotplug.txt):

maxcpus=n Restrict boot time cpus to n. Say if you have 4 cpus, using
maxcpus=2 will only boot 2. You can choose to bring the other cpus later online, read FAQ's for more info.

Looks like someone already documented this (IMO broken) behavior. I didn't find further info in the FAQs.

Quoted 4 lines

In any case, I was just trying to see why the simple removal of the setup_max_cpus check in acpi_processor_add() wasn't enough to expose the cpuidle directories under the new cpus.. and while debugging that, I came up with this patch. I don't mind if this doesn't get picked up.

Quoted 2 lines

Right, the usecase of why somebody would like to online new cpus beyond maxcpus doesn't look all that solid anyway. So I am OK with leaving the code as it is now.

In the end this is a debug option, I expect everybody is aware of that. Yep, let's just leave it...

Quoted 59 lines · Thomas Renninger

Quoted 28 lines · Srivatsa S. Bhat

Quoted 26 lines · Thomas Renninger

Quoted 11 lines · Srivatsa S. Bhat

Daniel Lezcano noticed that after booting with maxcpus=X, if we online the remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then for the newly onlined cpus, the cpuidle directory is not found under /sys/devices/system/cpu/cpuY.

Partly, the reason for this is that acpi restricts the initialization to cpus within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is used to restrict the number of cpus brought up during boot. That doesn't mean that we should hard restrict the bring up of the remaining cpus later on.

Sorry, but IMO it exaclty does mean that (adding more general lists for further comments).

If you can online more cores than maxcpus= via sysfs, this sounds like a bug. Not the other way around.

Compare with Documentation/kernel-parameters.txt:
maxcpus= [SMP] Maximum number of processors that an SMP kernel
should make use of. maxcpus=n : n >= 0 limits the kernel to using 'n' processors. n=0 is a special case, it is equivalent to "nosmp", which also disables the IO APIC.

Quoted 7 lines

Quoted 1 line

Chances that you run into more problems are high.

Right, I agree on that. So, IMHO, maxcpus=X doesn't mean that the kernel must and should forbid any new cpus from coming online, but in the interest of avoiding problems/complications in some obscure paths, I guess it makes sense to avoid onlining new cpus beyond maxcpus.

Yep, for such reasons:
- That nobody realizes this to be useful and makes use of it in a productive environment
- If I see maxcpus=X in a bugreport's dmesg command line,
I want to be sure that's true.
- To enforce that things work as documented

Wow, after looking a bit into this I found (Documentation/cpu-hotplug.txt):

maxcpus=n Restrict boot time cpus to n. Say if you have 4 cpus, using
maxcpus=2 will only boot 2. You can choose to bring the other cpus later online, read FAQ's for more info.

Looks like someone already documented this (IMO broken) behavior. I didn't find further info in the FAQs.

Quoted 4 lines

In any case, I was just trying to see why the simple removal of the setup_max_cpus check in acpi_processor_add() wasn't enough to expose the cpuidle directories under the new cpus.. and while debugging that, I came up with this patch. I don't mind if this doesn't get picked up.

Quoted 5 lines

Quoted 2 lines

Right, the usecase of why somebody would like to online new cpus beyond maxcpus doesn't look all that solid anyway. So I am OK with leaving the code as it is now.

In the end this is a debug option, I expect everybody is aware of that. Yep, let's just leave it...

In this case, let's remove the intel_idle_cpu_init stuff in acpi_cpu_soft_notify, no ?

Quoted 66 lines · Daniel Lezcano

Quoted 64 lines · Thomas Renninger

Quoted 28 lines · Srivatsa S. Bhat

Quoted 26 lines · Thomas Renninger

Quoted 11 lines · Srivatsa S. Bhat

Daniel Lezcano noticed that after booting with maxcpus=X, if we online the remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then for the newly onlined cpus, the cpuidle directory is not found under /sys/devices/system/cpu/cpuY.

Partly, the reason for this is that acpi restricts the initialization to cpus within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is used to restrict the number of cpus brought up during boot. That doesn't mean that we should hard restrict the bring up of the remaining cpus later on.

Sorry, but IMO it exaclty does mean that (adding more general lists for further comments).

If you can online more cores than maxcpus= via sysfs, this sounds like a bug. Not the other way around.

Compare with Documentation/kernel-parameters.txt:
maxcpus= [SMP] Maximum number of processors that an SMP kernel
should make use of. maxcpus=n : n >= 0 limits the kernel to using 'n' processors. n=0 is a special case, it is equivalent to "nosmp", which also disables the IO APIC.

Quoted 7 lines

Quoted 1 line

Chances that you run into more problems are high.

Right, I agree on that. So, IMHO, maxcpus=X doesn't mean that the kernel must and should forbid any new cpus from coming online, but in the interest of avoiding problems/complications in some obscure paths, I guess it makes sense to avoid onlining new cpus beyond maxcpus.

Yep, for such reasons:
- That nobody realizes this to be useful and makes use of it in a productive environment
- If I see maxcpus=X in a bugreport's dmesg command line,
I want to be sure that's true.
- To enforce that things work as documented

Wow, after looking a bit into this I found (Documentation/cpu-hotplug.txt):

maxcpus=n Restrict boot time cpus to n. Say if you have 4 cpus, using
maxcpus=2 will only boot 2. You can choose to bring the other cpus later online, read FAQ's for more info.

Looks like someone already documented this (IMO broken) behavior. I didn't find further info in the FAQs.

Quoted 7 lines

In any case, I was just trying to see why the simple removal of the setup_max_cpus check in acpi_processor_add() wasn't enough to expose the cpuidle directories under the new cpus.. and while debugging that, I came up with this patch. I don't mind if this doesn't get picked up.

Right, the usecase of why somebody would like to online new cpus beyond maxcpus doesn't look all that solid anyway. So I am OK with leaving the code as it is now.

In the end this is a debug option, I expect everybody is aware of that. Yep, let's just leave it...

Quoted 3 lines

In this case, let's remove the intel_idle_cpu_init stuff in acpi_cpu_soft_notify, no ?

Why? And how would that help? The intel_idle_cpu_init() call is essential if intel_idle driver is being used instead of acpi idle.

Quoted 2 lines · Srivatsa S. Bhat

Quoted 1 line · Daniel Lezcano

On 06/26/2012 11:29 AM, Thomas Renninger wrote:

Quoted 7 lines

Quoted 4 lines

In this case, let's remove the intel_idle_cpu_init stuff in acpi_cpu_soft_notify, no ?

Why? And how would that help? The intel_idle_cpu_init() call is essential if intel_idle driver is being used instead of acpi idle.

AFAIU, this code is not called after onlining a cpu greater than maxcpus and Thomas thinks that system with cpu hotplug at runtime are not sold.

The problem I see with this code is acpi and intel-idle are tied together now. I would like to break this dependency and use the notifier to handle the cpu hotplug directly in intel-idle.

It is hard to test my patch as there is not such system and maxcpus is not correctly handled here. I can use your patch to test my patch but anyway ... I am just asking if that would make sense to remove this portion of code instead :)

If we want to keep this code untouched, I can try my patch and maybe Thomas will agreed to test it also on a cpu-online-runtime-system if he has one.

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. In order to make it work, the notifier must be initialized in the right order, acpi then intel_idle. This is done in the Makefile. This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.

Signed-off-by Daniel Lezcano <[email protected]>

---
 drivers/Makefile                |    3 ++-
 drivers/acpi/processor_driver.c |    7 -------
 drivers/idle/intel_idle.c       |   22 ++++++++++++++--------
 include/linux/cpuidle.h         |    7 -------
 4 files changed, 16 insertions(+), 23 deletions(-)

drivers/Makefile +0 -0

index 2ba29ff..a2454b8 100644

drivers/Makefile +0 -0

 obj-$(CONFIG_SFI)		+= sfi/
 # PnP must come after ACPI since it will eventually need to check if acpi
 # was used and do nothing if so

drivers/acpi/processor_driver.c +0 -7

index 0734086..8648b29 100644
@@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
 		 * Initialize missing things
 		 */
 		if (pr->flags.need_hotplug_init) {
-			struct cpuidle_driver *idle_driver =
-				cpuidle_get_driver();
-
 			printk(KERN_INFO "Will online and init hotplugged "
 			       "CPU: %d\n", pr->id);
 			WARN(acpi_processor_start(pr), "Failed to start CPU:"
 				" %d\n", pr->id);
 			pr->flags.need_hotplug_init = 0;
-			if (idle_driver && !strcmp(idle_driver->name,
-						   "intel_idle")) {
-				intel_idle_cpu_init(pr->id);
-			}
 		/* Normal CPU soft online event */
 		} else {
 			acpi_processor_ppc_has_changed(pr, 0);

drivers/idle/intel_idle.c +4 -2

index d0f59c3..4c36039 100644
@@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
 static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
 static int intel_idle(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv, int index);
+static int intel_idle_cpu_init(int cpu);
 
 static struct cpuidle_state *cpuidle_state_table;
 

@@ -302,22 +303,28 @@ static void __setup_broadcast_timer(void *arg)
 	clockevents_notify(reason, &cpu);
 }
 
-static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
-		unsigned long action, void *hcpu)
+static int cpu_hotplug_notify(struct notifier_block *n,
+			      unsigned long action, void *hcpu)
 {
 	int hotcpu = (unsigned long)hcpu;
+	struct cpuidle_device *dev;

drivers/idle/intel_idle.c +2 -2

 		break;
 	}
 	return NOTIFY_OK;
 }
 
-static struct notifier_block setup_broadcast_notifier = {
-	.notifier_call = setup_broadcast_cpuhp_notify,
+static struct notifier_block cpu_hotplug_notifier = {
+	.notifier_call = cpu_hotplug_notify,
 };
 
 static void auto_demotion_disable(void *dummy)

drivers/idle/intel_idle.c +1 -2

 	}
 
 	pr_debug(PREFIX "v" INTEL_IDLE_VERSION

@@ -494,7 +501,7 @@ static int intel_idle_cpuidle_driver_init(void)
  * allocate, initialize, register cpuidle_devices
  * @cpu: cpu/core to initialize
  */
-int intel_idle_cpu_init(int cpu)
+static int intel_idle_cpu_init(int cpu)
 {
 	int cstate;
 	struct cpuidle_device *dev;

@@ -539,7 +546,6 @@ int intel_idle_cpu_init(int cpu)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
 
 static int __init intel_idle_init(void)
 {

Hi,

I agree that such a dependency between 2 modules is not nice. But your patch will have bad side-effects (see comments embedded below).

Quoted 25 lines · Daniel Lezcano

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. In order to make it work, the notifier must be initialized in the right order, acpi then intel_idle. This is done in the Makefile. This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.

Signed-off-by Daniel Lezcano <[email protected]>

---
 drivers/Makefile                |    3 ++-
 drivers/acpi/processor_driver.c |    7 -------
 drivers/idle/intel_idle.c       |   22 ++++++++++++++--------
 include/linux/cpuidle.h         |    7 -------
 4 files changed, 16 insertions(+), 23 deletions(-)

drivers/Makefile +0 -0

index 2ba29ff..a2454b8 100644

drivers/Makefile obj-$(CONFIG_PCI) += pci/ · line 17

Quoted 7 lines

drivers/Makefile +2 -1

@@ -12,8 +12,9 @@ obj-$(CONFIG_PCI)		+= pci/
 obj-$(CONFIG_PARISC)		+= parisc/
 obj-$(CONFIG_RAPIDIO)		+= rapidio/
 obj-y				+= video/
-obj-y				+= idle/
+# acpi must come before idle for initialization
 obj-$(CONFIG_ACPI)		+= acpi/
+obj-y				+= idle/

This breaks intel_idle.
Loading order defines which one comes first and is used: intel_idle or ACPI processor cpuidle driver.
With above, one would get acpi_idle cpuidle driver if both are compiled in, instead of the intel_idle one.

Why exactly is this necessary, couldn't it just work?

Quoted 8 lines

drivers/Makefile +2 -2

-static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
-		unsigned long action, void *hcpu)
+static int cpu_hotplug_notify(struct notifier_block *n,
+			      unsigned long action, void *hcpu)
 {

drivers/Makefile +1 -0

 	int hotcpu = (unsigned long)hcpu;
+	struct cpuidle_device *dev;

drivers/Makefile

Quoted 9 lines

drivers/Makefile +5 -0

 	switch (action & 0xf) {
 	case CPU_ONLINE:
 		smp_call_function_single(hotcpu, __setup_broadcast_timer,
 			(void *)true, 1);
+
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
+		if (!dev->registered)
+			intel_idle_cpu_init(hotcpu);
+

A small comment why this can happen and needs to be done (real hotplugged cpu case) might help here later.

drivers/Makefile static int intel_idle_probe(void) · line 417

Quoted 7 lines

static void auto_demotion_disable(void *dummy)

drivers/Makefile +1 -1

@@ -407,7 +414,7 @@ static int intel_idle_probe(void)
 		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
 	else {
 		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
-		register_cpu_notifier(&setup_broadcast_notifier);
+		register_cpu_notifier(&cpu_hotplug_notifier);

The notifier always has to be registered now, not only in:
if (boot_cpu_has(X86_FEATURE_ARAT)) /* Always Reliable APIC Timer */
case.

Quoted 20 lines

drivers/Makefile +1 -2

 	pr_debug(PREFIX "v" INTEL_IDLE_VERSION

@@ -494,7 +501,7 @@ static int intel_idle_cpuidle_driver_init(void)
  * allocate, initialize, register cpuidle_devices
  * @cpu: cpu/core to initialize
  */
-int intel_idle_cpu_init(int cpu)
+static int intel_idle_cpu_init(int cpu)
 {
 	int cstate;
 	struct cpuidle_device *dev;

@@ -539,7 +546,6 @@ int intel_idle_cpu_init(int cpu)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
 
 static int __init intel_idle_init(void)
 {

@@ -583,7 +589,7 @@ static void __exit intel_idle_exit(void)
 

drivers/Makefile static void __exit intel_idle_exit(void) · line 592

Quoted 4 lines

drivers/Makefile +1 -1

 	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
 		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
-		unregister_cpu_notifier(&setup_broadcast_notifier);
+		unregister_cpu_notifier(&cpu_hotplug_notifier);

Same.

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.

Signed-off-by Daniel Lezcano <[email protected]>

---
 drivers/acpi/processor_driver.c |    7 ------
 drivers/idle/intel_idle.c       |   41 +++++++++++++++++++++++++-------------
 include/linux/cpuidle.h         |    7 ------
 3 files changed, 27 insertions(+), 28 deletions(-)

drivers/acpi/processor_driver.c +0 -7

index 0734086..8648b29 100644
@@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
 		 * Initialize missing things
 		 */
 		if (pr->flags.need_hotplug_init) {
-			struct cpuidle_driver *idle_driver =
-				cpuidle_get_driver();
-
 			printk(KERN_INFO "Will online and init hotplugged "
 			       "CPU: %d\n", pr->id);
 			WARN(acpi_processor_start(pr), "Failed to start CPU:"
 				" %d\n", pr->id);
 			pr->flags.need_hotplug_init = 0;
-			if (idle_driver && !strcmp(idle_driver->name,
-						   "intel_idle")) {
-				intel_idle_cpu_init(pr->id);
-			}
 		/* Normal CPU soft online event */
 		} else {
 			acpi_processor_ppc_has_changed(pr, 0);

drivers/idle/intel_idle.c +27 -14

index d0f59c3..fe95d54 100644
@@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
 static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
 static int intel_idle(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv, int index);
+static int intel_idle_cpu_init(int cpu);
 
 static struct cpuidle_state *cpuidle_state_table;
 

@@ -302,22 +303,35 @@ static void __setup_broadcast_timer(void *arg)
 	clockevents_notify(reason, &cpu);
 }
 
-static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
-		unsigned long action, void *hcpu)
+static int cpu_hotplug_notify(struct notifier_block *n,
+			      unsigned long action, void *hcpu)
 {
 	int hotcpu = (unsigned long)hcpu;
+	struct cpuidle_device *dev;
 
 	switch (action & 0xf) {
 	case CPU_ONLINE:
-		smp_call_function_single(hotcpu, __setup_broadcast_timer,
-			(void *)true, 1);
+
+		if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
+			smp_call_function_single(hotcpu, __setup_broadcast_timer,
+						 (void *)true, 1);
+
+		/*
+		 * Some systems can hotplug a cpu at runtime after
+		 * the kernel has booted, we have to initialize the
+		 * driver in this case
+		 */
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
+		if (!dev->registered)
+			intel_idle_cpu_init(hotcpu);
+
 		break;
 	}
 	return NOTIFY_OK;
 }
 
-static struct notifier_block setup_broadcast_notifier = {
-	.notifier_call = setup_broadcast_cpuhp_notify,
+static struct notifier_block cpu_hotplug_notifier = {
+	.notifier_call = cpu_hotplug_notify,
 };
 
 static void auto_demotion_disable(void *dummy)

@@ -405,10 +419,10 @@ static int intel_idle_probe(void)
 
 	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
 		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
-	else {
+	else
 		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
-		register_cpu_notifier(&setup_broadcast_notifier);
-	}
+
+	register_cpu_notifier(&cpu_hotplug_notifier);
 
 	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
 		" model 0x%X\n", boot_cpu_data.x86_model);

@@ -494,7 +508,7 @@ static int intel_idle_cpuidle_driver_init(void)
  * allocate, initialize, register cpuidle_devices
  * @cpu: cpu/core to initialize
  */
-int intel_idle_cpu_init(int cpu)
+static int intel_idle_cpu_init(int cpu)
 {
 	int cstate;
 	struct cpuidle_device *dev;

@@ -539,7 +553,6 @@ int intel_idle_cpu_init(int cpu)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
 
 static int __init intel_idle_init(void)
 {

@@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void)
 	intel_idle_cpuidle_devices_uninit();
 	cpuidle_unregister_driver(&intel_idle_driver);
 
-	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
+
+	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
 		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
-		unregister_cpu_notifier(&setup_broadcast_notifier);
-	}
+	unregister_cpu_notifier(&cpu_hotplug_notifier);
 
 	return;
 }

include/linux/cpuidle.h +0 -7

index 5ab7183..66d7e0d 100644
@@ -213,14 +213,7 @@ struct cpuidle_governor {
 extern int cpuidle_register_governor(struct cpuidle_governor *gov);
 extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
 
-#ifdef CONFIG_INTEL_IDLE
-extern int intel_idle_cpu_init(int cpu);
 #else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
-#endif
-
-#else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
 
 static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 {return 0;}

Quoted 10 lines · Daniel Lezcano

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.

Quoted 1 line

Signed-off-by Daniel Lezcano <[email protected]>

Looks good to me.

Regards,
Srivatsa S. Bhat

Quoted 148 lines

---
 drivers/acpi/processor_driver.c |    7 ------
 drivers/idle/intel_idle.c       |   41 +++++++++++++++++++++++++-------------
 include/linux/cpuidle.h         |    7 ------
 3 files changed, 27 insertions(+), 28 deletions(-)

drivers/acpi/processor_driver.c +0 -7

index 0734086..8648b29 100644
@@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
 		 * Initialize missing things
 		 */
 		if (pr->flags.need_hotplug_init) {
-			struct cpuidle_driver *idle_driver =
-				cpuidle_get_driver();
-
 			printk(KERN_INFO "Will online and init hotplugged "
 			       "CPU: %d\n", pr->id);
 			WARN(acpi_processor_start(pr), "Failed to start CPU:"
 				" %d\n", pr->id);
 			pr->flags.need_hotplug_init = 0;
-			if (idle_driver && !strcmp(idle_driver->name,
-						   "intel_idle")) {
-				intel_idle_cpu_init(pr->id);
-			}
 		/* Normal CPU soft online event */
 		} else {
 			acpi_processor_ppc_has_changed(pr, 0);

drivers/idle/intel_idle.c +27 -14

index d0f59c3..fe95d54 100644
@@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
 static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
 static int intel_idle(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv, int index);
+static int intel_idle_cpu_init(int cpu);
 
 static struct cpuidle_state *cpuidle_state_table;
 

@@ -302,22 +303,35 @@ static void __setup_broadcast_timer(void *arg)
 	clockevents_notify(reason, &cpu);
 }
 
-static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
-		unsigned long action, void *hcpu)
+static int cpu_hotplug_notify(struct notifier_block *n,
+			      unsigned long action, void *hcpu)
 {
 	int hotcpu = (unsigned long)hcpu;
+	struct cpuidle_device *dev;
 
 	switch (action & 0xf) {
 	case CPU_ONLINE:
-		smp_call_function_single(hotcpu, __setup_broadcast_timer,
-			(void *)true, 1);
+
+		if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
+			smp_call_function_single(hotcpu, __setup_broadcast_timer,
+						 (void *)true, 1);
+
+		/*
+		 * Some systems can hotplug a cpu at runtime after
+		 * the kernel has booted, we have to initialize the
+		 * driver in this case
+		 */
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
+		if (!dev->registered)
+			intel_idle_cpu_init(hotcpu);
+
 		break;
 	}
 	return NOTIFY_OK;
 }
 
-static struct notifier_block setup_broadcast_notifier = {
-	.notifier_call = setup_broadcast_cpuhp_notify,
+static struct notifier_block cpu_hotplug_notifier = {
+	.notifier_call = cpu_hotplug_notify,
 };
 
 static void auto_demotion_disable(void *dummy)

@@ -405,10 +419,10 @@ static int intel_idle_probe(void)
 
 	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
 		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
-	else {
+	else
 		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
-		register_cpu_notifier(&setup_broadcast_notifier);
-	}
+
+	register_cpu_notifier(&cpu_hotplug_notifier);
 
 	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
 		" model 0x%X\n", boot_cpu_data.x86_model);

@@ -494,7 +508,7 @@ static int intel_idle_cpuidle_driver_init(void)
  * allocate, initialize, register cpuidle_devices
  * @cpu: cpu/core to initialize
  */
-int intel_idle_cpu_init(int cpu)
+static int intel_idle_cpu_init(int cpu)
 {
 	int cstate;
 	struct cpuidle_device *dev;

@@ -539,7 +553,6 @@ int intel_idle_cpu_init(int cpu)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
 
 static int __init intel_idle_init(void)
 {

@@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void)
 	intel_idle_cpuidle_devices_uninit();
 	cpuidle_unregister_driver(&intel_idle_driver);
 
-	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
+
+	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
 		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
-		unregister_cpu_notifier(&setup_broadcast_notifier);
-	}
+	unregister_cpu_notifier(&cpu_hotplug_notifier);
 
 	return;
 }

include/linux/cpuidle.h +0 -7

index 5ab7183..66d7e0d 100644
@@ -213,14 +213,7 @@ struct cpuidle_governor {
 extern int cpuidle_register_governor(struct cpuidle_governor *gov);
 extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
 
-#ifdef CONFIG_INTEL_IDLE
-extern int intel_idle_cpu_init(int cpu);
 #else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
-#endif
-
-#else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
 
 static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 {return 0;}

Quoted 11 lines · Srivatsa S. Bhat

Quoted 10 lines · Daniel Lezcano

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.

Quoted 4 lines

Quoted 1 line

Signed-off-by Daniel Lezcano <[email protected]>

Looks good to me.

Thanks for the review Srivatsa.

Shall I consider it as an acked-by ?

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.

Signed-off-by Daniel Lezcano <[email protected]>

---
 drivers/acpi/processor_driver.c |    7 ------
 drivers/idle/intel_idle.c       |   41 +++++++++++++++++++++++++-------------
 include/linux/cpuidle.h         |    7 ------
 3 files changed, 27 insertions(+), 28 deletions(-)

drivers/acpi/processor_driver.c +0 -7

index 0734086..8648b29 100644
@@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
 		 * Initialize missing things
 		 */
 		if (pr->flags.need_hotplug_init) {
-			struct cpuidle_driver *idle_driver =
-				cpuidle_get_driver();
-
 			printk(KERN_INFO "Will online and init hotplugged "
 			       "CPU: %d\n", pr->id);
 			WARN(acpi_processor_start(pr), "Failed to start CPU:"
 				" %d\n", pr->id);
 			pr->flags.need_hotplug_init = 0;
-			if (idle_driver && !strcmp(idle_driver->name,
-						   "intel_idle")) {
-				intel_idle_cpu_init(pr->id);
-			}
 		/* Normal CPU soft online event */
 		} else {
 			acpi_processor_ppc_has_changed(pr, 0);

drivers/idle/intel_idle.c +27 -14

index d0f59c3..fe95d54 100644
@@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
 static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
 static int intel_idle(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv, int index);
+static int intel_idle_cpu_init(int cpu);
 
 static struct cpuidle_state *cpuidle_state_table;
 

@@ -302,22 +303,35 @@ static void __setup_broadcast_timer(void *arg)
 	clockevents_notify(reason, &cpu);
 }
 
-static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
-		unsigned long action, void *hcpu)
+static int cpu_hotplug_notify(struct notifier_block *n,
+			      unsigned long action, void *hcpu)
 {
 	int hotcpu = (unsigned long)hcpu;
+	struct cpuidle_device *dev;
 
 	switch (action & 0xf) {
 	case CPU_ONLINE:
-		smp_call_function_single(hotcpu, __setup_broadcast_timer,
-			(void *)true, 1);
+
+		if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
+			smp_call_function_single(hotcpu, __setup_broadcast_timer,
+						 (void *)true, 1);
+
+		/*
+		 * Some systems can hotplug a cpu at runtime after
+		 * the kernel has booted, we have to initialize the
+		 * driver in this case
+		 */
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
+		if (!dev->registered)
+			intel_idle_cpu_init(hotcpu);
+
 		break;
 	}
 	return NOTIFY_OK;
 }
 
-static struct notifier_block setup_broadcast_notifier = {
-	.notifier_call = setup_broadcast_cpuhp_notify,
+static struct notifier_block cpu_hotplug_notifier = {
+	.notifier_call = cpu_hotplug_notify,
 };
 
 static void auto_demotion_disable(void *dummy)

@@ -405,10 +419,10 @@ static int intel_idle_probe(void)
 
 	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
 		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
-	else {
+	else
 		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
-		register_cpu_notifier(&setup_broadcast_notifier);
-	}
+
+	register_cpu_notifier(&cpu_hotplug_notifier);
 
 	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
 		" model 0x%X\n", boot_cpu_data.x86_model);

@@ -494,7 +508,7 @@ static int intel_idle_cpuidle_driver_init(void)
  * allocate, initialize, register cpuidle_devices
  * @cpu: cpu/core to initialize
  */
-int intel_idle_cpu_init(int cpu)
+static int intel_idle_cpu_init(int cpu)
 {
 	int cstate;
 	struct cpuidle_device *dev;

@@ -539,7 +553,6 @@ int intel_idle_cpu_init(int cpu)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
 
 static int __init intel_idle_init(void)
 {

@@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void)
 	intel_idle_cpuidle_devices_uninit();
 	cpuidle_unregister_driver(&intel_idle_driver);
 
-	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
+
+	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
 		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
-		unregister_cpu_notifier(&setup_broadcast_notifier);
-	}
+	unregister_cpu_notifier(&cpu_hotplug_notifier);
 
 	return;
 }

include/linux/cpuidle.h +0 -7

index 5ab7183..66d7e0d 100644
@@ -213,14 +213,7 @@ struct cpuidle_governor {
 extern int cpuidle_register_governor(struct cpuidle_governor *gov);
 extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
 
-#ifdef CONFIG_INTEL_IDLE
-extern int intel_idle_cpu_init(int cpu);
 #else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
-#endif
-
-#else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
 
 static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 {return 0;}

Quoted 10 lines · Daniel Lezcano

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.

Quoted 1 line

Signed-off-by Daniel Lezcano <[email protected]>

This one looks good to me too.

Acked-by Rafael J. Wysocki <[email protected]>

Len, are you going to take it?

Rafael

Quoted 148 lines

---
 drivers/acpi/processor_driver.c |    7 ------
 drivers/idle/intel_idle.c       |   41 +++++++++++++++++++++++++-------------
 include/linux/cpuidle.h         |    7 ------
 3 files changed, 27 insertions(+), 28 deletions(-)

drivers/acpi/processor_driver.c +0 -7

index 0734086..8648b29 100644
@@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
 		 * Initialize missing things
 		 */
 		if (pr->flags.need_hotplug_init) {
-			struct cpuidle_driver *idle_driver =
-				cpuidle_get_driver();
-
 			printk(KERN_INFO "Will online and init hotplugged "
 			       "CPU: %d\n", pr->id);
 			WARN(acpi_processor_start(pr), "Failed to start CPU:"
 				" %d\n", pr->id);
 			pr->flags.need_hotplug_init = 0;
-			if (idle_driver && !strcmp(idle_driver->name,
-						   "intel_idle")) {
-				intel_idle_cpu_init(pr->id);
-			}
 		/* Normal CPU soft online event */
 		} else {
 			acpi_processor_ppc_has_changed(pr, 0);

drivers/idle/intel_idle.c +27 -14

index d0f59c3..fe95d54 100644
@@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
 static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
 static int intel_idle(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv, int index);
+static int intel_idle_cpu_init(int cpu);
 
 static struct cpuidle_state *cpuidle_state_table;
 

@@ -302,22 +303,35 @@ static void __setup_broadcast_timer(void *arg)
 	clockevents_notify(reason, &cpu);
 }
 
-static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
-		unsigned long action, void *hcpu)
+static int cpu_hotplug_notify(struct notifier_block *n,
+			      unsigned long action, void *hcpu)
 {
 	int hotcpu = (unsigned long)hcpu;
+	struct cpuidle_device *dev;
 
 	switch (action & 0xf) {
 	case CPU_ONLINE:
-		smp_call_function_single(hotcpu, __setup_broadcast_timer,
-			(void *)true, 1);
+
+		if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
+			smp_call_function_single(hotcpu, __setup_broadcast_timer,
+						 (void *)true, 1);
+
+		/*
+		 * Some systems can hotplug a cpu at runtime after
+		 * the kernel has booted, we have to initialize the
+		 * driver in this case
+		 */
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
+		if (!dev->registered)
+			intel_idle_cpu_init(hotcpu);
+
 		break;
 	}
 	return NOTIFY_OK;
 }
 
-static struct notifier_block setup_broadcast_notifier = {
-	.notifier_call = setup_broadcast_cpuhp_notify,
+static struct notifier_block cpu_hotplug_notifier = {
+	.notifier_call = cpu_hotplug_notify,
 };
 
 static void auto_demotion_disable(void *dummy)

@@ -405,10 +419,10 @@ static int intel_idle_probe(void)
 
 	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
 		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
-	else {
+	else
 		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
-		register_cpu_notifier(&setup_broadcast_notifier);
-	}
+
+	register_cpu_notifier(&cpu_hotplug_notifier);
 
 	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
 		" model 0x%X\n", boot_cpu_data.x86_model);

@@ -494,7 +508,7 @@ static int intel_idle_cpuidle_driver_init(void)
  * allocate, initialize, register cpuidle_devices
  * @cpu: cpu/core to initialize
  */
-int intel_idle_cpu_init(int cpu)
+static int intel_idle_cpu_init(int cpu)
 {
 	int cstate;
 	struct cpuidle_device *dev;

@@ -539,7 +553,6 @@ int intel_idle_cpu_init(int cpu)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
 
 static int __init intel_idle_init(void)
 {

@@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void)
 	intel_idle_cpuidle_devices_uninit();
 	cpuidle_unregister_driver(&intel_idle_driver);
 
-	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
+
+	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
 		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
-		unregister_cpu_notifier(&setup_broadcast_notifier);
-	}
+	unregister_cpu_notifier(&cpu_hotplug_notifier);
 
 	return;
 }

include/linux/cpuidle.h +0 -7

index 5ab7183..66d7e0d 100644
@@ -213,14 +213,7 @@ struct cpuidle_governor {
 extern int cpuidle_register_governor(struct cpuidle_governor *gov);
 extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
 
-#ifdef CONFIG_INTEL_IDLE
-extern int intel_idle_cpu_init(int cpu);
 #else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
-#endif
-
-#else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
 
 static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 {return 0;}

Quoted 11 lines · Rafael J. Wysocki

Quoted 10 lines · Daniel Lezcano

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.

Quoted 5 lines

Quoted 1 line

Signed-off-by Daniel Lezcano <[email protected]>

This one looks good to me too.

Acked-by Rafael J. Wysocki <[email protected]>

Thanks for the review Rafael.

Quoted 1 line

Len, are you going to take it?

Rafael, Len,

After the discussion [1], I put in place a tree at:

ssh://git.linaro.org/srv/git.linaro.org/git/people/dlezcano/cpuidle-next.git #cpuidle-next

I proposed this tree to group the cpuidle modifications and prevent the last minutes conflict when Len will apply them.

I also included the tree into linux-next for wider testing.

Quoted 29 lines · Daniel Lezcano

Quoted 11 lines · Rafael J. Wysocki

Quoted 10 lines · Daniel Lezcano

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. This patch has the benefit of encapsulating the intel_idle driver and remove some exported functions.

Quoted 5 lines

Quoted 1 line

Signed-off-by Daniel Lezcano <[email protected]>

This one looks good to me too.

Acked-by Rafael J. Wysocki <[email protected]>

Thanks for the review Rafael.

Quoted 1 line

Len, are you going to take it?

Rafael, Len,

After the discussion [1], I put in place a tree at:

ssh://git.linaro.org/srv/git.linaro.org/git/people/dlezcano/cpuidle-next.git #cpuidle-next

Quoted 4 lines

I proposed this tree to group the cpuidle modifications and prevent the last minutes conflict when Len will apply them.

I also included the tree into linux-next for wider testing.

I can't speak for Len, but I'm not sure he'll like this.

Anyway, you seem to have the same material as Len in that tree, won't there be any conflicts in linux-next?

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. In order to make it work, the notifier must be initialized in the right order, acpi then intel_idle. This is done in the Makefile.

This patch has the benefit of
encapsulating the intel_idle driver and remove some exported functions.

Signed-off-by Daniel Lezcano <[email protected]>

---
 drivers/Makefile                |    3 ++-
 drivers/acpi/processor_driver.c |    7 -------
 drivers/idle/intel_idle.c       |   22 ++++++++++++++--------
 include/linux/cpuidle.h         |    7 -------
 4 files changed, 16 insertions(+), 23 deletions(-)

drivers/Makefile +0 -0

index 2ba29ff..a2454b8 100644

drivers/acpi/processor_driver.c +0 -7

index 0734086..8648b29 100644
@@ -427,18 +427,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
 		 * Initialize missing things
 		 */
 		if (pr->flags.need_hotplug_init) {
-			struct cpuidle_driver *idle_driver =
-				cpuidle_get_driver();
-
 			printk(KERN_INFO "Will online and init hotplugged "
 			       "CPU: %d\n", pr->id);
 			WARN(acpi_processor_start(pr), "Failed to start CPU:"
 				" %d\n", pr->id);
 			pr->flags.need_hotplug_init = 0;
-			if (idle_driver && !strcmp(idle_driver->name,
-						   "intel_idle")) {
-				intel_idle_cpu_init(pr->id);
-			}
 		/* Normal CPU soft online event */
 		} else {
 			acpi_processor_ppc_has_changed(pr, 0);

drivers/idle/intel_idle.c +14 -8

index d0f59c3..4c36039 100644
@@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
 static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
 static int intel_idle(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv, int index);
+static int intel_idle_cpu_init(int cpu);
 
 static struct cpuidle_state *cpuidle_state_table;
 

@@ -302,22 +303,28 @@ static void __setup_broadcast_timer(void *arg)
 	clockevents_notify(reason, &cpu);
 }
 
-static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
-		unsigned long action, void *hcpu)
+static int cpu_hotplug_notify(struct notifier_block *n,
+			      unsigned long action, void *hcpu)
 {
 	int hotcpu = (unsigned long)hcpu;
+	struct cpuidle_device *dev;
 
 	switch (action & 0xf) {
 	case CPU_ONLINE:
 		smp_call_function_single(hotcpu, __setup_broadcast_timer,
 			(void *)true, 1);
+
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
+		if (!dev->registered)
+			intel_idle_cpu_init(hotcpu);
+
 		break;
 	}
 	return NOTIFY_OK;
 }
 
-static struct notifier_block setup_broadcast_notifier = {
-	.notifier_call = setup_broadcast_cpuhp_notify,
+static struct notifier_block cpu_hotplug_notifier = {
+	.notifier_call = cpu_hotplug_notify,
 };
 
 static void auto_demotion_disable(void *dummy)

@@ -407,7 +414,7 @@ static int intel_idle_probe(void)
 		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
 	else {
 		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
-		register_cpu_notifier(&setup_broadcast_notifier);
+		register_cpu_notifier(&cpu_hotplug_notifier);
 	}
 
 	pr_debug(PREFIX "v" INTEL_IDLE_VERSION

@@ -494,7 +501,7 @@ static int intel_idle_cpuidle_driver_init(void)
  * allocate, initialize, register cpuidle_devices
  * @cpu: cpu/core to initialize
  */
-int intel_idle_cpu_init(int cpu)
+static int intel_idle_cpu_init(int cpu)
 {
 	int cstate;
 	struct cpuidle_device *dev;

@@ -539,7 +546,6 @@ int intel_idle_cpu_init(int cpu)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
 
 static int __init intel_idle_init(void)
 {

@@ -583,7 +589,7 @@ static void __exit intel_idle_exit(void)
 
 	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
 		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
-		unregister_cpu_notifier(&setup_broadcast_notifier);
+		unregister_cpu_notifier(&cpu_hotplug_notifier);
 	}
 
 	return;

include/linux/cpuidle.h +0 -7

index 5ab7183..66d7e0d 100644
@@ -213,14 +213,7 @@ struct cpuidle_governor {
 extern int cpuidle_register_governor(struct cpuidle_governor *gov);
 extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
 
-#ifdef CONFIG_INTEL_IDLE
-extern int intel_idle_cpu_init(int cpu);
 #else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
-#endif
-
-#else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
 
 static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 {return 0;}

Quoted 9 lines · Daniel Lezcano

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. In order to make it work, the notifier must be initialized in the right order, acpi then intel_idle. This is done in the Makefile.

There is a much better way of doing this. See below.

Quoted 4 lines

This patch has the benefit of
encapsulating the intel_idle driver and remove some exported functions.

Nice :)

Quoted 18 lines

Signed-off-by Daniel Lezcano <[email protected]>

---
 drivers/Makefile                |    3 ++-
 drivers/acpi/processor_driver.c |    7 -------
 drivers/idle/intel_idle.c       |   22 ++++++++++++++--------
 include/linux/cpuidle.h         |    7 -------
 4 files changed, 16 insertions(+), 23 deletions(-)

drivers/Makefile +1 -1

index 2ba29ff..a2454b8 100644
@@ -12,8 +12,9 @@ obj-$(CONFIG_PCI)		+= pci/
 obj-$(CONFIG_PARISC)		+= parisc/
 obj-$(CONFIG_RAPIDIO)		+= rapidio/
 obj-y				+= video/
-obj-y				+= idle/
+# acpi must come before idle for initialization

drivers/Makefile obj-$(CONFIG_PCI) += pci/ · line 20

Quoted 5 lines

drivers/Makefile +1 -0

 obj-$(CONFIG_ACPI)		+= acpi/
+obj-y				+= idle/
 obj-$(CONFIG_SFI)		+= sfi/
 # PnP must come after ACPI since it will eventually need to check if acpi
 # was used and do nothing if so

OK, so all you are trying to do here is ensure that the intel idle related notifier runs _after_ the acpi related one.

Quoted 38 lines · Srivatsa S. Bhat

Quoted 9 lines · Daniel Lezcano

When the system is booted with some cpus offline, the idle driver is not initialized. When a cpu is set online, the acpi code call the intel idle init function. Unfortunately this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the notifier of intel_idle. In order to make it work, the notifier must be initialized in the right order, acpi then intel_idle. This is done in the Makefile.

There is a much better way of doing this. See below.

Quoted 4 lines

This patch has the benefit of
encapsulating the intel_idle driver and remove some exported functions.

Nice :)

Quoted 18 lines

Signed-off-by Daniel Lezcano <[email protected]>

---
 drivers/Makefile                |    3 ++-
 drivers/acpi/processor_driver.c |    7 -------
 drivers/idle/intel_idle.c       |   22 ++++++++++++++--------
 include/linux/cpuidle.h         |    7 -------
 4 files changed, 16 insertions(+), 23 deletions(-)

drivers/Makefile +1 -1

index 2ba29ff..a2454b8 100644
@@ -12,8 +12,9 @@ obj-$(CONFIG_PCI)		+= pci/
 obj-$(CONFIG_PARISC)		+= parisc/
 obj-$(CONFIG_RAPIDIO)		+= rapidio/
 obj-y				+= video/
-obj-y				+= idle/
+# acpi must come before idle for initialization

Quoted 8 lines

drivers/Makefile obj-$(CONFIG_PCI) += pci/ · line 20

Quoted 5 lines

drivers/Makefile +1 -0

 obj-$(CONFIG_ACPI)		+= acpi/
+obj-y				+= idle/
 obj-$(CONFIG_SFI)		+= sfi/
 # PnP must come after ACPI since it will eventually need to check if acpi
 # was used and do nothing if so

OK, so all you are trying to do here is ensure that the intel idle related notifier runs _after_ the acpi related one.

I might oversee something, if you have concerns, please point me to it. If it's all about keeping the order of excuting these functions:
acpi_processor_start(pr)
and
intel_idle_cpu_init()
There should be no need for it. Intel idle is pretty separated.

Quoted 71 lines · Srivatsa S. Bhat

Quoted 69 lines · Daniel Lezcano

Quoted 59 lines · Thomas Renninger

Quoted 28 lines · Srivatsa S. Bhat

Quoted 26 lines · Thomas Renninger

Quoted 11 lines · Srivatsa S. Bhat

Daniel Lezcano noticed that after booting with maxcpus=X, if we online the remaining cpus by writing: echo 1 > /sys/devices/system/cpu/cpuY/online, then for the newly onlined cpus, the cpuidle directory is not found under /sys/devices/system/cpu/cpuY.

Partly, the reason for this is that acpi restricts the initialization to cpus within the maxcpus limit. (See commit 75cbfb9 "ACPI: Do not try to set up acpi processor stuff on cores exceeding maxcpus="). The maxcpus= kernel parameter is used to restrict the number of cpus brought up during boot. That doesn't mean that we should hard restrict the bring up of the remaining cpus later on.

Sorry, but IMO it exaclty does mean that (adding more general lists for further comments).

If you can online more cores than maxcpus= via sysfs, this sounds like a bug. Not the other way around.

Compare with Documentation/kernel-parameters.txt:
maxcpus= [SMP] Maximum number of processors that an SMP kernel
should make use of. maxcpus=n : n >= 0 limits the kernel to using 'n' processors. n=0 is a special case, it is equivalent to "nosmp", which also disables the IO APIC.

Quoted 7 lines

Quoted 1 line

Chances that you run into more problems are high.

Right, I agree on that. So, IMHO, maxcpus=X doesn't mean that the kernel must and should forbid any new cpus from coming online, but in the interest of avoiding problems/complications in some obscure paths, I guess it makes sense to avoid onlining new cpus beyond maxcpus.

Yep, for such reasons:
- That nobody realizes this to be useful and makes use of it in a productive environment
- If I see maxcpus=X in a bugreport's dmesg command line,
I want to be sure that's true.
- To enforce that things work as documented

Wow, after looking a bit into this I found (Documentation/cpu-hotplug.txt):

maxcpus=n Restrict boot time cpus to n. Say if you have 4 cpus, using
maxcpus=2 will only boot 2. You can choose to bring the other cpus later online, read FAQ's for more info.

Looks like someone already documented this (IMO broken) behavior. I didn't find further info in the FAQs.

Quoted 4 lines

In any case, I was just trying to see why the simple removal of the setup_max_cpus check in acpi_processor_add() wasn't enough to expose the cpuidle directories under the new cpus.. and while debugging that, I came up with this patch. I don't mind if this doesn't get picked up.

Quoted 5 lines

Quoted 2 lines

Right, the usecase of why somebody would like to online new cpus beyond maxcpus doesn't look all that solid anyway. So I am OK with leaving the code as it is now.

In the end this is a debug option, I expect everybody is aware of that. Yep, let's just leave it...

In this case, let's remove the intel_idle_cpu_init stuff in acpi_cpu_soft_notify, no ?

Quoted 2 lines

Why? And how would that help? The intel_idle_cpu_init() call is essential if intel_idle driver is being used instead of acpi idle.

AFAIU, this code is not called after onlining a cpu greater than maxcpus and Thomas thinks that system with cpu hotplug at runtime are not sold.

The problem I see with this code is acpi and intel-idle are tied together now. I would like to break this dependency and use the notifier to handle the cpu hotplug directly in intel-idle.

It is hard to test my patch as there is not such system and maxcpus is not correctly handled here. I can use your patch to test my patch but anyway ... I am just asking if that would make sense to remove this portion of code instead :)

If we want to keep this code untouched, I can try my patch and maybe Thomas will agreed to test it also on a cpu-online-runtime-system if he has one.