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 daniel.lezcano@linaro.org Acked-by: Srivatsa S. Bhat srivatsa.bhat@linux.vnet.ibm.com Acked-by: Rafael J. Wysocki rjw@sisk.pl --- drivers/acpi/processor_driver.c | 7 ------ drivers/idle/intel_idle.c | 41 +++++++++++++++++++++++++------------- include/linux/cpuidle.h | 7 ------ 3 files changed, 27 insertions(+), 28 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index 13103aeb..7048b97 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -429,18 +429,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); diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index d0f59c3..fe95d54 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -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; } diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 8684a0d..a6b3f2e 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -207,14 +207,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;}
We have the state index passed as parameter to the 'enter' function. Most of the drivers assign their 'enter' functions several times in the cpuidle_state structure, as we have the index, we can delegate to the driver to handle their own callback array.
That will have the benefit of removing multiple lines of code in the different drivers.
In order to smoothly modify the driver, the 'enter' function are in the driver structure and in the cpuidle state structure. That will let the time to modify the different drivers one by one. So the 'cpuidle_enter' function checks if the 'enter' callback is assigned in the driver structure and use it, otherwise it invokes the 'enter' assigned to the cpuidle_state.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/cpuidle/cpuidle.c | 4 +++- include/linux/cpuidle.h | 1 + 2 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 0132706..c064144 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -46,7 +46,9 @@ static inline int cpuidle_enter(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { struct cpuidle_state *target_state = &drv->states[index]; - return target_state->enter(dev, drv, index); + + return drv->enter ? drv->enter(dev, drv, index) : + target_state->enter(dev, drv, index); }
static inline int cpuidle_enter_tk(struct cpuidle_device *dev, diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index a6b3f2e..4d816c7 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -131,6 +131,7 @@ struct cpuidle_driver { struct cpuidle_state states[CPUIDLE_STATE_MAX]; int state_count; int safe_state_index; + int (*enter)(struct cpuidle_device *, struct cpuidle_driver *, int); };
#ifdef CONFIG_CPU_IDLE
On Thursday, July 05, 2012, Daniel Lezcano wrote:
We have the state index passed as parameter to the 'enter' function. Most of the drivers assign their 'enter' functions several times in the cpuidle_state structure, as we have the index, we can delegate to the driver to handle their own callback array.
That will have the benefit of removing multiple lines of code in the different drivers.
Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason. Among other things, this was to avoid recurrence in callbacks - please see acpi_idle_enter_bm() for example.
Now, if .enter() is moved to the driver structure, it will have to be an all-purpose complicated routine calling itself recursively at least in some cases. I'm not quite convinced that would be an improvement.
On the other hand, I don't see anything wrong with setting several callback pointers to the same routine.
Thanks, Rafael
On 07/05/2012 10:38 PM, Rafael J. Wysocki wrote:
On Thursday, July 05, 2012, Daniel Lezcano wrote:
We have the state index passed as parameter to the 'enter' function. Most of the drivers assign their 'enter' functions several times in the cpuidle_state structure, as we have the index, we can delegate to the driver to handle their own callback array.
That will have the benefit of removing multiple lines of code in the different drivers.
Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason. Among other things, this was to avoid recurrence in callbacks - please see acpi_idle_enter_bm() for example.
Now, if .enter() is moved to the driver structure, it will have to be an all-purpose complicated routine calling itself recursively at least in some cases. I'm not quite convinced that would be an improvement.
On the other hand, I don't see anything wrong with setting several callback pointers to the same routine.
Deepthi sent a few months ago a patch moving the per-cpu cpuidle_state to a single structure stored in the driver. The drivers were modified and cleanup to take into account this modification.
We saw some regressions like for example the 'disable' which were not per cpu and has been moved to the cpuidle_state_usage (and IMHO it is a workaround more than a real fix). And now we have new architectures (tegra3, big.LITTLE) with different latencies per cpu. Logically we should revert Deepthi's patches but from my POV, going back and forth is not a good solution (also we have to undo all modifications done in the different drivers).
The main purpose of all these cleanup patches are to move out all non-data information from the cpuidle_state structure in order to add a new api which could be 'cpuidle_register_cpu_latency(int cpu, struct cpuidle_latencies latencies)'.
For this specific patch, the 'enter' function for all the drivers is not used [1] and one of the driver is about to use a single function [2]. So we have only one driver is a couple of functions for this which can be replaced by an array of callbacks in the driver itself as we have the index.
[1] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012355.html [2] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012399.html
On Friday, July 06, 2012, Daniel Lezcano wrote:
On 07/05/2012 10:38 PM, Rafael J. Wysocki wrote:
On Thursday, July 05, 2012, Daniel Lezcano wrote:
We have the state index passed as parameter to the 'enter' function. Most of the drivers assign their 'enter' functions several times in the cpuidle_state structure, as we have the index, we can delegate to the driver to handle their own callback array.
That will have the benefit of removing multiple lines of code in the different drivers.
Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason. Among other things, this was to avoid recurrence in callbacks - please see acpi_idle_enter_bm() for example.
Now, if .enter() is moved to the driver structure, it will have to be an all-purpose complicated routine calling itself recursively at least in some cases. I'm not quite convinced that would be an improvement.
On the other hand, I don't see anything wrong with setting several callback pointers to the same routine.
Deepthi sent a few months ago a patch moving the per-cpu cpuidle_state to a single structure stored in the driver. The drivers were modified and cleanup to take into account this modification.
We saw some regressions like for example the 'disable' which were not per cpu and has been moved to the cpuidle_state_usage (and IMHO it is a workaround more than a real fix). And now we have new architectures (tegra3, big.LITTLE) with different latencies per cpu. Logically we should revert Deepthi's patches but from my POV, going back and forth is not a good solution (also we have to undo all modifications done in the different drivers).
The main purpose of all these cleanup patches are to move out all non-data information from the cpuidle_state structure in order to add a new api which could be 'cpuidle_register_cpu_latency(int cpu, struct cpuidle_latencies latencies)'.
Well, my question is: what specifically is wrong with having callback pointers in struct cpuidle_state in case when that structure is known to be common for all the CPUs in the system?
Sure, there are systems in which that is not the case, but then why should we complicate the simplest case in order to address them?
For this specific patch, the 'enter' function for all the drivers is not used [1] and one of the driver is about to use a single function [2]. So we have only one driver is a couple of functions for this which can be replaced by an array of callbacks in the driver itself as we have the index.
[1] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012355.html
I agree with Deepthi.
[2] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012399.html
And that is fine and dandy as long as the OMAP people want that. Evidently, that is not wanted by everybody and if it had been, the cpuidle subsystem would have been designed differently.
I really would like that thing to be reconsidered before we proceed with these changes.
Thanks, Rafael
Hi Daniel,
On Friday 06 July 2012 04:28 PM, Daniel Lezcano wrote:
The main purpose of all these cleanup patches are to move out all non-data information from the cpuidle_state structure in order to add a new api which could be 'cpuidle_register_cpu_latency(int cpu, struct cpuidle_latencies latencies)'.
So are there any technical difficulties in adding such an api with the non-data information being part of cpuidle_state? Or its just that you think the already duplicated non-data information (per state) is going to be duplicated much more (per state per cpu) in most cases.
regards, Rajendra
On 07/10/2012 10:29 AM, Rajendra Nayak wrote:
Hi Daniel,
On Friday 06 July 2012 04:28 PM, Daniel Lezcano wrote:
The main purpose of all these cleanup patches are to move out all non-data information from the cpuidle_state structure in order to add a new api which could be 'cpuidle_register_cpu_latency(int cpu, struct cpuidle_latencies latencies)'.
So are there any technical difficulties in adding such an api with the non-data information being part of cpuidle_state? Or its just that you think the already duplicated non-data information (per state) is going to be duplicated much more (per state per cpu) in most cases.
Yes, it is possible. I am already working on an alternate patchset I will post soon, but I don't like to add more things in a subsystem before cleaning up the code when it makes sense.
When I looked at the cpuidle code, I noticed this 'enter' field was used for the same function. With per cpu latency and using the cpuidle_state structure, that means duplication of this field. For an x86_64 architecture with 16 cores and 4 C-states, that is 512 bytes of memory instead of 8 bytes.
That is the same for example the 'disable' field which could be stored in the 'flags' field as a bit instead of the moving it to the 'stats' structure with "unsigned long long".
As the architectures tend to add more cores [1][2], that means more memory consumption. In a very near future, we will have 100 cores in a cpu. Assuming we have 4 C-states, that is 100 * 4 * 8 = 3200 bytes of memory used for a single function. IMHO, the kernel shouldn't assume the user must add more memory on the system.
Anyway, I will post a patchset to use the cpuidle_state per cpu.
Thanks -- Daniel
[1] http://www.engadget.com/2007/02/11/intel-demonstrates-80-core-processor/ [2] http://www.tilera.com/products/processors/TILE-Gx_Family
As the architectures tend to add more cores [1][2], that means more memory consumption. In a very near future, we will have 100 cores in a cpu. Assuming we have 4 C-states, that is 100 * 4 * 8 = 3200 bytes of memory used for a single function. IMHO, the kernel shouldn't assume the user must add more memory on the system.
I completely agree.
Anyway, I will post a patchset to use the cpuidle_state per cpu.
Maybe what you were doing wasn't clearly evident with the existing cpuidle core, but once its scaled to add a per cpu state, it might look more meaningful. So if you post your patches to add cpuidle state per cpu, it should certainly give more insight into why this cleanup makes sense.
Thanks -- Daniel
[1] http://www.engadget.com/2007/02/11/intel-demonstrates-80-core-processor/ [2] http://www.tilera.com/products/processors/TILE-Gx_Family
The 'enter_dead' function is only used for processor_idle.c and the same function is used several times. We fall into the same abuse with the multiple callbacks for the same function.
This patch fixes that by moving the 'enter_dead' function to the driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added to handle the callback conditional invokation.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/acpi/processor_idle.c | 7 +++---- drivers/cpuidle/cpuidle.c | 4 ++-- include/linux/cpuidle.h | 26 ++++++++++++++++++++------ 3 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 6d9ec3e..33310fb 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -1024,6 +1024,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev, struct cpuidle_driver acpi_idle_driver = { .name = "acpi_idle", .owner = THIS_MODULE, + .enter_dead = acpi_idle_play_dead, };
/** @@ -1132,16 +1133,14 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr) case ACPI_STATE_C1: if (cx->entry_method == ACPI_CSTATE_FFH) state->flags |= CPUIDLE_FLAG_TIME_VALID; - + state->flags |= CPUIDLE_FLAG_DEAD_VALID; state->enter = acpi_idle_enter_c1; - state->enter_dead = acpi_idle_play_dead; drv->safe_state_index = count; break;
case ACPI_STATE_C2: - state->flags |= CPUIDLE_FLAG_TIME_VALID; + state->flags |= CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_DEAD_VALID; state->enter = acpi_idle_enter_simple; - state->enter_dead = acpi_idle_play_dead; drv->safe_state_index = count; break;
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index c064144..7dcfa45 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -81,14 +81,14 @@ int cpuidle_play_dead(void) for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { struct cpuidle_state *s = &drv->states[i];
- if (s->power_usage < power_usage && s->enter_dead) { + if (s->power_usage < power_usage && (s->flags & CPUIDLE_FLAG_DEAD_VALID)) { power_usage = s->power_usage; dead_state = i; } }
if (dead_state != -1) - return drv->states[dead_state].enter_dead(dev, dead_state); + return drv->enter_dead(dev, dead_state);
return -ENODEV; } diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 4d816c7..730e12e 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -52,14 +52,25 @@ struct cpuidle_state { int (*enter) (struct cpuidle_device *dev, struct cpuidle_driver *drv, int index); - - int (*enter_dead) (struct cpuidle_device *dev, int index); };
-/* Idle State Flags */ -#define CPUIDLE_FLAG_TIME_VALID (0x01) /* is residency time measurable? */ - -#define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000) +/******************************************************************************** + * Idle State Flags: * + * CPUIDLE_FLAG_TIME_VALID : the drivers could measure the state residency * + * time and store it in the 'target_residency'field. * + * This flag will tell the cpuidle core to use this * + * value to compute an accurate prediction. * + * * + * CPUIDLE_FLAG_COUPLED : state applies to multiple cpus * + * * + * CPUIDLE_FLAG_DEAD_VALID : the driver may want to deal with dead cpus, tell * + * the cpuidle core the specified state can use the * + * enter_dead function. * + * * + *******************************************************************************/ +#define CPUIDLE_FLAG_TIME_VALID (0x01) +#define CPUIDLE_FLAG_COUPLED (0x02) +#define CPUIDLE_FLAG_DEAD_VALID (0x04)
/** * cpuidle_get_statedata - retrieves private driver state data @@ -132,6 +143,9 @@ struct cpuidle_driver { int state_count; int safe_state_index; int (*enter)(struct cpuidle_device *, struct cpuidle_driver *, int); +#ifdef CONFIG_ACPI + int (*enter_dead)(struct cpuidle_device *, int); +#endif };
#ifdef CONFIG_CPU_IDLE
On Thursday, July 05, 2012, Daniel Lezcano wrote:
The 'enter_dead' function is only used for processor_idle.c and the same function is used several times. We fall into the same abuse with the multiple callbacks for the same function.
This isn't abuse, mind you. This is a normal practice.
This patch fixes that by moving the 'enter_dead' function to the driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added to handle the callback conditional invokation.
And how does that improve things?
Rafael
On 07/05/2012 10:40 PM, Rafael J. Wysocki wrote:
On Thursday, July 05, 2012, Daniel Lezcano wrote:
The 'enter_dead' function is only used for processor_idle.c and the same function is used several times. We fall into the same abuse with the multiple callbacks for the same function.
This isn't abuse, mind you. This is a normal practice.
Well, that depends :)
I agree adding a callback per state is nice and flexible but if it is not used, it is a waste of memory, even if it is 32 bytes.
This patch fixes that by moving the 'enter_dead' function to the driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added to handle the callback conditional invokation.
And how does that improve things?
In order to check if the play_dead is enabled for a specific state, we check if the pointer is set. As it has been moved to a single function, we need to add a flag to replace this check. That is not an improvement, it replace a check by another check.
On Friday, July 06, 2012, Daniel Lezcano wrote:
On 07/05/2012 10:40 PM, Rafael J. Wysocki wrote:
On Thursday, July 05, 2012, Daniel Lezcano wrote:
The 'enter_dead' function is only used for processor_idle.c and the same function is used several times. We fall into the same abuse with the multiple callbacks for the same function.
This isn't abuse, mind you. This is a normal practice.
Well, that depends :)
I agree adding a callback per state is nice and flexible
Yes, it is.
but if it is not used, it is a waste of memory, even if it is 32 bytes.
32 bits, perhaps? And how many of those are there in the whole system, actually? Is this a number that actually matters?
This patch fixes that by moving the 'enter_dead' function to the driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added to handle the callback conditional invokation.
And how does that improve things?
In order to check if the play_dead is enabled for a specific state, we check if the pointer is set. As it has been moved to a single function, we need to add a flag to replace this check. That is not an improvement,
Well, exactly.
it replace a check by another check.
Sorry, but I don't really see the point.
Thanks, Rafael
Move this specific flag to the header file.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/idle/intel_idle.c | 8 -------- include/linux/cpuidle.h | 16 +++++++++++++--- 2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index fe95d54..3f0eb07 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -101,14 +101,6 @@ static int intel_idle_cpu_init(int cpu); static struct cpuidle_state *cpuidle_state_table;
/* - * Set this flag for states where the HW flushes the TLB for us - * and so we don't need cross-calls to keep it consistent. - * If this flag is set, SW flushes the TLB, so even if the - * HW doesn't do the flushing, this flag is safe to use. - */ -#define CPUIDLE_FLAG_TLB_FLUSHED 0x10000 - -/* * States are indexed by the cstate number, * which is also the index into the MWAIT hint array. * Thus C0 is a dummy. diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 730e12e..be150c9 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -67,10 +67,20 @@ struct cpuidle_state { * the cpuidle core the specified state can use the * * enter_dead function. * * * + * CPUIDLE_FLAG_TLB_FLUSHED : Set this flag for states where the HW flushes the * + * TLB for us and so we don't need cross-calls to * + * keep it consistent. If this flag is set, SW * + * flushes the TLB, so even if the HW doesn't do the * + * flushing, this flag is safe to use. * + * * *******************************************************************************/ -#define CPUIDLE_FLAG_TIME_VALID (0x01) -#define CPUIDLE_FLAG_COUPLED (0x02) -#define CPUIDLE_FLAG_DEAD_VALID (0x04) +#define CPUIDLE_FLAG_TIME_VALID (0x01) +#define CPUIDLE_FLAG_COUPLED (0x02) +#define CPUIDLE_FLAG_DEAD_VALID (0x04) +#define CPUIDLE_FLAG_TLB_FLUSHED (0x08) + + +
/** * cpuidle_get_statedata - retrieves private driver state data
On Thursday, July 05, 2012, Daniel Lezcano wrote:
Move this specific flag to the header file.
The patch evidently does more than that.
Is it just a cleanup, or is there a functional reason for doing it?
Rafael
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/idle/intel_idle.c | 8 -------- include/linux/cpuidle.h | 16 +++++++++++++--- 2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index fe95d54..3f0eb07 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -101,14 +101,6 @@ static int intel_idle_cpu_init(int cpu); static struct cpuidle_state *cpuidle_state_table; /*
- Set this flag for states where the HW flushes the TLB for us
- and so we don't need cross-calls to keep it consistent.
- If this flag is set, SW flushes the TLB, so even if the
- HW doesn't do the flushing, this flag is safe to use.
- */
-#define CPUIDLE_FLAG_TLB_FLUSHED 0x10000
-/*
- States are indexed by the cstate number,
- which is also the index into the MWAIT hint array.
- Thus C0 is a dummy.
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 730e12e..be150c9 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -67,10 +67,20 @@ struct cpuidle_state {
the cpuidle core the specified state can use the *enter_dead function. **
- CPUIDLE_FLAG_TLB_FLUSHED : Set this flag for states where the HW flushes the *
TLB for us and so we don't need cross-calls to *
keep it consistent. If this flag is set, SW *
flushes the TLB, so even if the HW doesn't do the *
flushing, this flag is safe to use. ********************************************************************************/
*-#define CPUIDLE_FLAG_TIME_VALID (0x01) -#define CPUIDLE_FLAG_COUPLED (0x02) -#define CPUIDLE_FLAG_DEAD_VALID (0x04) +#define CPUIDLE_FLAG_TIME_VALID (0x01) +#define CPUIDLE_FLAG_COUPLED (0x02) +#define CPUIDLE_FLAG_DEAD_VALID (0x04) +#define CPUIDLE_FLAG_TLB_FLUSHED (0x08)
/**
- cpuidle_get_statedata - retrieves private driver state data
On 07/05/2012 10:43 PM, Rafael J. Wysocki wrote:
On Thursday, July 05, 2012, Daniel Lezcano wrote:
Move this specific flag to the header file.
The patch evidently does more than that.
Is it just a cleanup, or is there a functional reason for doing it?
It is just a cleanup.
Thanks for reviewing the patches.
-- Daniel
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/idle/intel_idle.c | 8 -------- include/linux/cpuidle.h | 16 +++++++++++++--- 2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index fe95d54..3f0eb07 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -101,14 +101,6 @@ static int intel_idle_cpu_init(int cpu); static struct cpuidle_state *cpuidle_state_table; /*
- Set this flag for states where the HW flushes the TLB for us
- and so we don't need cross-calls to keep it consistent.
- If this flag is set, SW flushes the TLB, so even if the
- HW doesn't do the flushing, this flag is safe to use.
- */
-#define CPUIDLE_FLAG_TLB_FLUSHED 0x10000
-/*
- States are indexed by the cstate number,
- which is also the index into the MWAIT hint array.
- Thus C0 is a dummy.
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 730e12e..be150c9 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -67,10 +67,20 @@ struct cpuidle_state {
the cpuidle core the specified state can use the *enter_dead function. **
- CPUIDLE_FLAG_TLB_FLUSHED : Set this flag for states where the HW flushes the *
TLB for us and so we don't need cross-calls to *
keep it consistent. If this flag is set, SW *
flushes the TLB, so even if the HW doesn't do the *
flushing, this flag is safe to use. ********************************************************************************/
*-#define CPUIDLE_FLAG_TIME_VALID (0x01) -#define CPUIDLE_FLAG_COUPLED (0x02) -#define CPUIDLE_FLAG_DEAD_VALID (0x04) +#define CPUIDLE_FLAG_TIME_VALID (0x01) +#define CPUIDLE_FLAG_COUPLED (0x02) +#define CPUIDLE_FLAG_DEAD_VALID (0x04) +#define CPUIDLE_FLAG_TLB_FLUSHED (0x08)
/**
- cpuidle_get_statedata - retrieves private driver state data
On Friday, July 06, 2012, Daniel Lezcano wrote:
On 07/05/2012 10:43 PM, Rafael J. Wysocki wrote:
On Thursday, July 05, 2012, Daniel Lezcano wrote:
Move this specific flag to the header file.
The patch evidently does more than that.
Is it just a cleanup, or is there a functional reason for doing it?
It is just a cleanup.
In that case, can you please say that in the changelog? I have no objections to the patch itself, but the changelog is clearly substandard.
Thanks for reviewing the patches.
No problem.
Thanks, Rafael
On Thursday, July 05, 2012, Daniel Lezcano wrote:
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 daniel.lezcano@linaro.org Acked-by: Srivatsa S. Bhat srivatsa.bhat@linux.vnet.ibm.com Acked-by: Rafael J. Wysocki rjw@sisk.pl
OK, I'm taking this one.
Thanks, Rafael
drivers/acpi/processor_driver.c | 7 ------ drivers/idle/intel_idle.c | 41 +++++++++++++++++++++++++------------- include/linux/cpuidle.h | 7 ------ 3 files changed, 27 insertions(+), 28 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c index 13103aeb..7048b97 100644 --- a/drivers/acpi/processor_driver.c +++ b/drivers/acpi/processor_driver.c @@ -429,18 +429,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);}diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index d0f59c3..fe95d54 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -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; } diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 8684a0d..a6b3f2e 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -207,14 +207,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;}
-
Daniel Lezcano -
Rafael J. Wysocki -
Rajendra Nayak