Hi Rafael/Daniel,
This is a small cleanup patchset for CPUIdle which can go in 3.13 if it looks okay to you guys..
Mostly trivial patches but few are doing good/significant changes. Tested on my thinkpad with suspend/resume and didn't found any broken stuff with it.
I a not very sure about this patch (As I don't know about all aspects of CPUIdle framework): cpuidle: don't call poll_idle_init() for every cpu
-- viresh
Viresh Kumar (21): cpuidle: fix indentation of cpumask cpuidle: Fix comments in cpuidle core cpuidle: make __cpuidle_get_cpu_driver() inline cpuidle: make __cpuidle_device_init() return void cpuidle: make __cpuidle_driver_init() return void cpuidle: rearrange code in __cpuidle_driver_init() cpuidle: rearrange __cpuidle_register_device() to keep minimal exit points cpuidle: use cpuidle_disabled() instead of "off" cpuidle: merge two if() statements for checking error cases cpuidle: reduce code duplication inside cpuidle_idle_call() cpuidle: replace multiline statements with single line in cpuidle_idle_call() cpuidle: call cpuidle_get_driver() from after taking cpuidle_driver_lock cpuidle: use drv instead of cpuidle_driver in show_current_driver() cpuidle: coupled: don't compare cpu masks unnecessarily cpuidle: free all state kobjects from cpuidle_free_state_kobj() cpuidle: avoid unnecessary kzalloc/free of struct cpuidle_device_kobj cpuidle: avoid unnecessary kzalloc/free of struct cpuidle_driver_kobj cpuidle: don't call poll_idle_init() for every cpu cpuidle: create list of registered drivers cpuidle: don't calculate time-diff if entered_state == 0 cpuidle: change governor from within cpuidle_replace_governor()
drivers/cpuidle/coupled.c | 9 +-- drivers/cpuidle/cpuidle.c | 95 +++++++------------------ drivers/cpuidle/driver.c | 171 ++++++++++++++++++++------------------------- drivers/cpuidle/governor.c | 24 +++---- drivers/cpuidle/sysfs.c | 74 +++++++------------- include/linux/cpuidle.h | 25 +++++-- 6 files changed, 161 insertions(+), 237 deletions(-)
cpumask is indented using spaces instead of tabs. Fix it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/cpuidle.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 781addc..c082425 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -114,7 +114,7 @@ struct cpuidle_driver { int safe_state_index;
/* the driver handles the cpus in cpumask */ - struct cpumask *cpumask; + struct cpumask *cpumask; };
#ifdef CONFIG_CPU_IDLE
Few comments in cpuidle core files have trivial mistakes. This patch fixes them.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/coupled.c | 2 +- drivers/cpuidle/cpuidle.c | 2 +- drivers/cpuidle/driver.c | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c index f8a8636..e952936 100644 --- a/drivers/cpuidle/coupled.c +++ b/drivers/cpuidle/coupled.c @@ -147,7 +147,7 @@ static cpumask_t cpuidle_coupled_poked; * has returned from this function, the barrier is immediately available for * reuse. * - * The atomic variable a must be initialized to 0 before any cpu calls + * The atomic variable must be initialized to 0 before any cpu calls * this function, will be reset to 0 before any cpu returns from this function. * * Must only be called from within a coupled idle state handler diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index d75040d..8827c02 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -516,7 +516,7 @@ int cpuidle_register(struct cpuidle_driver *drv,
#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED /* - * On multiplatform for ARM, the coupled idle states could + * On multiplatform for ARM, the coupled idle states could be * enabled in the kernel even if the cpuidle driver does not * use it. Note, coupled_cpus is a struct copy. */ diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 6e11701..ced1df6 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -56,7 +56,7 @@ static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) }
/** - * __cpuidle_set_driver - set per CPU driver variables the the given driver. + * __cpuidle_set_driver - set per CPU driver variables for the given driver. * @drv: a valid pointer to a struct cpuidle_driver * * For each CPU in the driver's cpumask, unset the registered driver per CPU @@ -132,7 +132,7 @@ static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) * cpuidle_setup_broadcast_timer - enable/disable the broadcast timer * @arg: a void pointer used to match the SMP cross call API * - * @arg is used as a value of type 'long' with on of the two values: + * @arg is used as a value of type 'long' with one of the two values: * - CLOCK_EVT_NOTIFY_BROADCAST_ON * - CLOCK_EVT_NOTIFY_BROADCAST_OFF * @@ -169,7 +169,7 @@ static int __cpuidle_driver_init(struct cpuidle_driver *drv) /* * Look for the timer stop flag in the different states, so that we know * if the broadcast timer has to be set up. The loop is in the reverse - * order, because usually on of the the deeper states has this flag set. + * order, because usually one of the deeper states have this flag set. */ for (i = drv->state_count - 1; i >= 0 ; i--) {
__cpuidle_get_cpu_driver() is a single line function and so deserves to be marked inline.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index ced1df6..25455e8 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -29,7 +29,7 @@ static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers); * Returns a pointer to struct cpuidle_driver or NULL if no driver has been * registered for @cpu. */ -static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) +static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) { return per_cpu(cpuidle_drivers, cpu); }
On 09/22/2013 03:20 AM, Viresh Kumar wrote:
__cpuidle_get_cpu_driver() is a single line function and so deserves to be marked inline.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index ced1df6..25455e8 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -29,7 +29,7 @@ static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
- Returns a pointer to struct cpuidle_driver or NULL if no driver has been
- registered for @cpu.
*/ -static struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) +static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) { return per_cpu(cpuidle_drivers, cpu); }
__cpuidle_device_init() doesn't return anything except zero and so doesn't really need a return type other than void.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 8827c02..211e504 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -358,12 +358,10 @@ static void __cpuidle_unregister_device(struct cpuidle_device *dev) module_put(drv->owner); }
-static int __cpuidle_device_init(struct cpuidle_device *dev) +static void __cpuidle_device_init(struct cpuidle_device *dev) { memset(dev->states_usage, 0, sizeof(dev->states_usage)); dev->last_residency = 0; - - return 0; }
/** @@ -410,9 +408,7 @@ int cpuidle_register_device(struct cpuidle_device *dev) if (dev->registered) goto out_unlock;
- ret = __cpuidle_device_init(dev); - if (ret) - goto out_unlock; + __cpuidle_device_init(dev);
ret = __cpuidle_register_device(dev); if (ret)
__cpuidle_driver_init() doesn't return anything except zero and so doesn't really need a return type other than void.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 25455e8..8df66c8 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -152,7 +152,7 @@ static void cpuidle_setup_broadcast_timer(void *arg) * * Returns 0 on success, a negative error code otherwise. */ -static int __cpuidle_driver_init(struct cpuidle_driver *drv) +static void __cpuidle_driver_init(struct cpuidle_driver *drv) { int i;
@@ -179,8 +179,6 @@ static int __cpuidle_driver_init(struct cpuidle_driver *drv) drv->bctimer = 1; break; } - - return 0; }
/** @@ -206,9 +204,7 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv) if (cpuidle_disabled()) return -ENODEV;
- ret = __cpuidle_driver_init(drv); - if (ret) - return ret; + __cpuidle_driver_init(drv);
ret = __cpuidle_set_driver(drv); if (ret)
On 09/22/2013 09:20 AM, Viresh Kumar wrote:
__cpuidle_driver_init() doesn't return anything except zero and so doesn't really need a return type other than void.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 25455e8..8df66c8 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -152,7 +152,7 @@ static void cpuidle_setup_broadcast_timer(void *arg)
- Returns 0 on success, a negative error code otherwise.
If you want to make it return void, you should also update the above comment.
*/ -static int __cpuidle_driver_init(struct cpuidle_driver *drv) +static void __cpuidle_driver_init(struct cpuidle_driver *drv) { int i; @@ -179,8 +179,6 @@ static int __cpuidle_driver_init(struct cpuidle_driver *drv) drv->bctimer = 1; break; }
- return 0; }
/** @@ -206,9 +204,7 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv) if (cpuidle_disabled()) return -ENODEV;
- ret = __cpuidle_driver_init(drv);
- if (ret)
return ret;
- __cpuidle_driver_init(drv);
ret = __cpuidle_set_driver(drv); if (ret)
On 23 September 2013 15:28, Hongbo Zhang hongbo.zhang@freescale.com wrote:
On 09/22/2013 09:20 AM, Viresh Kumar wrote:
__cpuidle_driver_init() doesn't return anything except zero and so doesn't really need a return type other than void.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 25455e8..8df66c8 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -152,7 +152,7 @@ static void cpuidle_setup_broadcast_timer(void *arg)
- Returns 0 on success, a negative error code otherwise.
If you want to make it return void, you should also update the above comment.
Thanks for the correction..
This is trivial patch that just reorders few statements in __cpuidle_driver_init() routine, so that we don't need both 'continue' and 'break' in the for loop. Functionally it shouldn't change anything.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/driver.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 8df66c8..6279e1c 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -172,12 +172,10 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) * order, because usually one of the deeper states have this flag set. */ for (i = drv->state_count - 1; i >= 0 ; i--) { - - if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP)) - continue; - - drv->bctimer = 1; - break; + if (drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP) { + drv->bctimer = 1; + break; + } } }
On 09/22/2013 03:20 AM, Viresh Kumar wrote:
This is trivial patch that just reorders few statements in __cpuidle_driver_init() routine, so that we don't need both 'continue' and 'break' in the for loop. Functionally it shouldn't change anything.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/driver.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 8df66c8..6279e1c 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -172,12 +172,10 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) * order, because usually one of the deeper states have this flag set. */ for (i = drv->state_count - 1; i >= 0 ; i--) {
if (!(drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP))continue;drv->bctimer = 1;break;
if (drv->states[i].flags & CPUIDLE_FLAG_TIMER_STOP) {drv->bctimer = 1;break;}}
Well, I don't have a strong opinion on that, it is "Schtroumpf Vert et Vert Schtroumpf" :) [1]
-- Daniel
[1] http://en.wikipedia.org/wiki/Schtroumpf_Vert_et_Vert_Schtroumpf
On 26 September 2013 03:10, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Well, I don't have a strong opinion on that, it is "Schtroumpf Vert et Vert Schtroumpf" :) [1]
:)
This patch rearranges __cpuidle_register_device() a bit in order to reduce the number of exit points of this function.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 211e504..8c91bad 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -383,13 +383,12 @@ static int __cpuidle_register_device(struct cpuidle_device *dev) list_add(&dev->device_list, &cpuidle_detected_devices);
ret = cpuidle_coupled_register_device(dev); - if (ret) { + if (ret) __cpuidle_unregister_device(dev); - return ret; - } + else + dev->registered = 1;
- dev->registered = 1; - return 0; + return ret; }
/**
On 09/22/2013 03:20 AM, Viresh Kumar wrote:
This patch rearranges __cpuidle_register_device() a bit in order to reduce the number of exit points of this function.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/cpuidle.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 211e504..8c91bad 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -383,13 +383,12 @@ static int __cpuidle_register_device(struct cpuidle_device *dev) list_add(&dev->device_list, &cpuidle_detected_devices); ret = cpuidle_coupled_register_device(dev);
- if (ret) {
- if (ret) __cpuidle_unregister_device(dev);
return ret;- }
- else
dev->registered = 1;
- dev->registered = 1;
- return 0;
- return ret;
}
There is no accounting for taste :)
I agree the patch concentrates more the return statement into a single place which conforms better to the kernel coding style.
We have a routine for getting value of "off", better call that instead of using "off" directly.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 8c91bad..aec9029 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -119,7 +119,7 @@ int cpuidle_idle_call(void) struct cpuidle_driver *drv; int next_state, entered_state;
- if (off) + if (cpuidle_disabled()) return -ENODEV;
if (!initialized)
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
We have a routine for getting value of "off", better call that instead of using "off" directly.
We are in the fast path, I am not sure invoking a function here is better than using directly the static variable.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/cpuidle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 8c91bad..aec9029 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -119,7 +119,7 @@ int cpuidle_idle_call(void) struct cpuidle_driver *drv; int next_state, entered_state;
- if (off)
- if (cpuidle_disabled()) return -ENODEV;
if (!initialized)
On 26 September 2013 03:22, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
We have a routine for getting value of "off", better call that instead of using "off" directly.
We are in the fast path, I am not sure invoking a function here is better than using directly the static variable.
I only did it for consistency as we have this routine specifically for reading value of "off" and so we better don't use off directly..
Probably we can make it static inline and move it into drivers/cpuidle/cpuidle.h?
On 09/26/2013 07:06 AM, Viresh Kumar wrote:
On 26 September 2013 03:22, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
We have a routine for getting value of "off", better call that instead of using "off" directly.
We are in the fast path, I am not sure invoking a function here is better than using directly the static variable.
I only did it for consistency as we have this routine specifically for reading value of "off" and so we better don't use off directly..
Probably we can make it static inline and move it into drivers/cpuidle/cpuidle.h?
If you move it to cpuidle.h, you will have to move the 'off' variable in the header hence increasing the scope of it.
Both return same error message and so better write them in a single line.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index aec9029..b8c63cb 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -119,10 +119,7 @@ int cpuidle_idle_call(void) struct cpuidle_driver *drv; int next_state, entered_state;
- if (cpuidle_disabled()) - return -ENODEV; - - if (!initialized) + if (cpuidle_disabled() || !initialized) return -ENODEV;
/* check if the device is ready */
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
Both return same error message and so better write them in a single line.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
modulo the comment on patch 08/21:
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/cpuidle.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index aec9029..b8c63cb 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -119,10 +119,7 @@ int cpuidle_idle_call(void) struct cpuidle_driver *drv; int next_state, entered_state;
- if (cpuidle_disabled())
return -ENODEV;- if (!initialized)
- if (cpuidle_disabled() || !initialized) return -ENODEV;
/* check if the device is ready */
We are doing this twice in cpuidle_idle_call() routine: drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP
Would be better if we actually store this in a local variable and use that. That would remove code duplication as well as make this piece of code run fast (in case compiler wasn't able to optimize it earlier)
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index b8c63cb..ed67e3c 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -118,6 +118,7 @@ int cpuidle_idle_call(void) struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices); struct cpuidle_driver *drv; int next_state, entered_state; + bool broadcast;
if (cpuidle_disabled() || !initialized) return -ENODEV; @@ -141,7 +142,9 @@ int cpuidle_idle_call(void)
trace_cpu_idle_rcuidle(next_state, dev->cpu);
- if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP) + broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP; + + if (broadcast) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
@@ -151,7 +154,7 @@ int cpuidle_idle_call(void) else entered_state = cpuidle_enter_state(dev, drv, next_state);
- if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP) + if (broadcast) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
We are doing this twice in cpuidle_idle_call() routine: drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP
Would be better if we actually store this in a local variable and use that. That would remove code duplication as well as make this piece of code run fast (in case compiler wasn't able to optimize it earlier)
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/cpuidle.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index b8c63cb..ed67e3c 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -118,6 +118,7 @@ int cpuidle_idle_call(void) struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices); struct cpuidle_driver *drv; int next_state, entered_state;
- bool broadcast;
if (cpuidle_disabled() || !initialized) return -ENODEV; @@ -141,7 +142,9 @@ int cpuidle_idle_call(void) trace_cpu_idle_rcuidle(next_state, dev->cpu);
- if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP)
- broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
- if (broadcast) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
@@ -151,7 +154,7 @@ int cpuidle_idle_call(void) else entered_state = cpuidle_enter_state(dev, drv, next_state);
- if (drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP)
- if (broadcast) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
Few statements in cpuidle_idle_call() are broken into multiple lines, whereas they can actually come in a single line. Convert those to single line.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index ed67e3c..43d5836 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -145,8 +145,7 @@ int cpuidle_idle_call(void) broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
if (broadcast) - clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, - &dev->cpu); + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
if (cpuidle_state_is_coupled(dev, drv, next_state)) entered_state = cpuidle_enter_state_coupled(dev, drv, @@ -155,8 +154,7 @@ int cpuidle_idle_call(void) entered_state = cpuidle_enter_state(dev, drv, next_state);
if (broadcast) - clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, - &dev->cpu); + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
Few statements in cpuidle_idle_call() are broken into multiple lines, whereas they can actually come in a single line. Convert those to single line.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I splitted these lines because they have 81 cols.
drivers/cpuidle/cpuidle.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index ed67e3c..43d5836 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -145,8 +145,7 @@ int cpuidle_idle_call(void) broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP; if (broadcast)
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,&dev->cpu);
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);if (cpuidle_state_is_coupled(dev, drv, next_state)) entered_state = cpuidle_enter_state_coupled(dev, drv, @@ -155,8 +154,7 @@ int cpuidle_idle_call(void) entered_state = cpuidle_enter_state(dev, drv, next_state); if (broadcast)
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT,&dev->cpu);
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
On 26 September 2013 03:33, Daniel Lezcano daniel.lezcano@linaro.org wrote:
I splitted these lines because they have 81 cols.
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,&dev->cpu);
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
This one comes in 80 columns
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT,&dev->cpu);
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
And this one in 79..
And so I did this change.. I have checked it again now..
On 09/26/2013 07:51 AM, Viresh Kumar wrote:
On 26 September 2013 03:33, Daniel Lezcano daniel.lezcano@linaro.org wrote:
I splitted these lines because they have 81 cols.
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER,&dev->cpu);
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);This one comes in 80 columns
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT,&dev->cpu);
clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);And this one in 79..
And so I did this change.. I have checked it again now..
Well I have 81 with Vi and 80 with Emacs :)
May be some tabs converted to spaces ...
Anyway if it fits 80 cols then it is ok.
On 26 September 2013 13:25, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Well I have 81 with Vi and 80 with Emacs :)
Hmm.. vim gave me 80 :) .. I am looking at bottom right corner where it gives: 148, 66-80 22%
cpuidle_driver_lock is taken correctly at most of the places but at few places calls to cpuidle_get_driver() are done from outside of this lock.
Fix them by calling cpuidle_get_driver() after taking cpuidle_driver_lock.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/driver.c | 3 ++- drivers/cpuidle/sysfs.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 6279e1c..7b2510a 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -340,10 +340,11 @@ struct cpuidle_driver *cpuidle_driver_ref(void) */ void cpuidle_driver_unref(void) { - struct cpuidle_driver *drv = cpuidle_get_driver(); + struct cpuidle_driver *drv;
spin_lock(&cpuidle_driver_lock);
+ drv = cpuidle_get_driver(); if (drv && !WARN_ON(drv->refcnt <= 0)) drv->refcnt--;
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index 8739cc0..a022393 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -52,9 +52,10 @@ static ssize_t show_current_driver(struct device *dev, char *buf) { ssize_t ret; - struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver(); + struct cpuidle_driver *cpuidle_driver;
spin_lock(&cpuidle_driver_lock); + cpuidle_driver = cpuidle_get_driver(); if (cpuidle_driver) ret = sprintf(buf, "%s\n", cpuidle_driver->name); else
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
cpuidle_driver_lock is taken correctly at most of the places but at few places calls to cpuidle_get_driver() are done from outside of this lock.
Fix them by calling cpuidle_get_driver() after taking cpuidle_driver_lock.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/driver.c | 3 ++- drivers/cpuidle/sysfs.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 6279e1c..7b2510a 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -340,10 +340,11 @@ struct cpuidle_driver *cpuidle_driver_ref(void) */ void cpuidle_driver_unref(void) {
- struct cpuidle_driver *drv = cpuidle_get_driver();
- struct cpuidle_driver *drv;
spin_lock(&cpuidle_driver_lock);
- drv = cpuidle_get_driver(); if (drv && !WARN_ON(drv->refcnt <= 0)) drv->refcnt--;
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index 8739cc0..a022393 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -52,9 +52,10 @@ static ssize_t show_current_driver(struct device *dev, char *buf) { ssize_t ret;
- struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();
- struct cpuidle_driver *cpuidle_driver;
spin_lock(&cpuidle_driver_lock);
- cpuidle_driver = cpuidle_get_driver(); if (cpuidle_driver) ret = sprintf(buf, "%s\n", cpuidle_driver->name); else
Instances of "struct cpuidle_driver *" are consistently named as "drv" in cpuidle core. Its broken only at one place: show_current_driver().
Fix it for consistency.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/sysfs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index a022393..e918b6d 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -52,12 +52,12 @@ static ssize_t show_current_driver(struct device *dev, char *buf) { ssize_t ret; - struct cpuidle_driver *cpuidle_driver; + struct cpuidle_driver *drv;
spin_lock(&cpuidle_driver_lock); - cpuidle_driver = cpuidle_get_driver(); - if (cpuidle_driver) - ret = sprintf(buf, "%s\n", cpuidle_driver->name); + drv = cpuidle_get_driver(); + if (drv) + ret = sprintf(buf, "%s\n", drv->name); else ret = sprintf(buf, "none\n"); spin_unlock(&cpuidle_driver_lock);
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
Instances of "struct cpuidle_driver *" are consistently named as "drv" in cpuidle core. Its broken only at one place: show_current_driver().
Fix it for consistency.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/sysfs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index a022393..e918b6d 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -52,12 +52,12 @@ static ssize_t show_current_driver(struct device *dev, char *buf) { ssize_t ret;
- struct cpuidle_driver *cpuidle_driver;
- struct cpuidle_driver *drv;
spin_lock(&cpuidle_driver_lock);
- cpuidle_driver = cpuidle_get_driver();
- if (cpuidle_driver)
ret = sprintf(buf, "%s\n", cpuidle_driver->name);
- drv = cpuidle_get_driver();
- if (drv)
ret = sprintf(buf, "%s\n", drv->name);
In cpuidle_coupled_register_device() we do following: if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus))) coupled->prevent++;
This is only required to be done when we are using 'coupled' from an existing cpuidle_device and not when we have just done this:
coupled->coupled_cpus = dev->coupled_cpus
So, move this compare statement to the right place.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/coupled.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c index e952936..19a89eb 100644 --- a/drivers/cpuidle/coupled.c +++ b/drivers/cpuidle/coupled.c @@ -642,6 +642,10 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev) other_dev = per_cpu(cpuidle_devices, cpu); if (other_dev && other_dev->coupled) { coupled = other_dev->coupled; + + if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, + &coupled->coupled_cpus))) + coupled->prevent++; goto have_coupled; } } @@ -655,9 +659,6 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev)
have_coupled: dev->coupled = coupled; - if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus))) - coupled->prevent++; - cpuidle_coupled_update_online_cpus(coupled);
coupled->refcnt++;
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
In cpuidle_coupled_register_device() we do following: if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus))) coupled->prevent++;
This is only required to be done when we are using 'coupled' from an existing cpuidle_device and not when we have just done this:
coupled->coupled_cpus = dev->coupled_cpus
So, move this compare statement to the right place.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Cc'ed Colin Cross.
drivers/cpuidle/coupled.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c index e952936..19a89eb 100644 --- a/drivers/cpuidle/coupled.c +++ b/drivers/cpuidle/coupled.c @@ -642,6 +642,10 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev) other_dev = per_cpu(cpuidle_devices, cpu); if (other_dev && other_dev->coupled) { coupled = other_dev->coupled;
if (WARN_ON(!cpumask_equal(&dev->coupled_cpus,&coupled->coupled_cpus)))coupled->prevent++; goto have_coupled;@@ -655,9 +659,6 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev) have_coupled: dev->coupled = coupled;
- if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus)))
coupled->prevent++;- cpuidle_coupled_update_online_cpus(coupled);
coupled->refcnt++;
On Sat, Sep 21, 2013 at 6:21 PM, Viresh Kumar viresh.kumar@linaro.org wrote:
In cpuidle_coupled_register_device() we do following: if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus))) coupled->prevent++;
This is only required to be done when we are using 'coupled' from an existing cpuidle_device and not when we have just done this:
coupled->coupled_cpus = dev->coupled_cpusSo, move this compare statement to the right place.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
I don't agree with this. This patch is a tiny optimization in code that is rarely called, and it moves a final sanity check somewhere that it might get missed if the code were later refactored.
drivers/cpuidle/coupled.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/cpuidle/coupled.c b/drivers/cpuidle/coupled.c index e952936..19a89eb 100644 --- a/drivers/cpuidle/coupled.c +++ b/drivers/cpuidle/coupled.c @@ -642,6 +642,10 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev) other_dev = per_cpu(cpuidle_devices, cpu); if (other_dev && other_dev->coupled) { coupled = other_dev->coupled;
if (WARN_ON(!cpumask_equal(&dev->coupled_cpus,&coupled->coupled_cpus)))coupled->prevent++; goto have_coupled; } }@@ -655,9 +659,6 @@ int cpuidle_coupled_register_device(struct cpuidle_device *dev)
have_coupled: dev->coupled = coupled;
if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &coupled->coupled_cpus)))coupled->prevent++;cpuidle_coupled_update_online_cpus(coupled); coupled->refcnt++;-- 1.7.12.rc2.18.g61b472e
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 26 September 2013 05:55, Colin Cross ccross@google.com wrote:
I don't agree with this. This patch is a tiny optimization in code that is rarely called, and it moves a final sanity check somewhere that it might get missed if the code were later refactored.
This is what we are doing for the first cpu of coupled-cpus: if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &dev->coupled_cpus))) coupled->prevent++;
i.e. comparing a variable to itself :)
And I believe my patch puts the sanity check at the right place (where we are using coupled from existing CPUs.. And that is where it should have been since the beginning)..
If people miss this during code re-factoring, then it would be even more stupid on the part of Author and Reviewer.. And if it still gets missed then this is not the only place where we need to worry about such stuff..
This is present everywhere in our code.. You can't really some part of code to some place and leave the other as-is.. The change is supposed to be more logical and so funny mistakes must be caught during reviews.
On Thu, Sep 26, 2013 at 1:36 AM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 26 September 2013 05:55, Colin Cross ccross@google.com wrote:
I don't agree with this. This patch is a tiny optimization in code that is rarely called, and it moves a final sanity check somewhere that it might get missed if the code were later refactored.
This is what we are doing for the first cpu of coupled-cpus: if (WARN_ON(!cpumask_equal(&dev->coupled_cpus, &dev->coupled_cpus))) coupled->prevent++;
i.e. comparing a variable to itself :)
And I believe my patch puts the sanity check at the right place (where we are using coupled from existing CPUs.. And that is where it should have been since the beginning)..
If people miss this during code re-factoring, then it would be even more stupid on the part of Author and Reviewer.. And if it still gets missed then this is not the only place where we need to worry about such stuff..
This is present everywhere in our code.. You can't really some part of code to some place and leave the other as-is.. The change is supposed to be more logical and so funny mistakes must be caught during reviews.
It's fine where it is. Once dev and coupled are both known, regardless of how they were found, it performs a final sanity check that nothing went wrong. Moving into a specific branch of the finding code defeats the purpose. Yes, it performs a useless check on the first cpu, but it keeps the code readable and maintainable, so it stays where it is. NAK.
Loop for states is currently present on callers side and so is replicated at several places. It would be better to move that inside cpuidle_free_state_kobj() instead.
This patch does it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/sysfs.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index e918b6d..ade31a9 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -378,12 +378,17 @@ static struct kobj_type ktype_state_cpuidle = { .release = cpuidle_state_sysfs_release, };
-static inline void cpuidle_free_state_kobj(struct cpuidle_device *device, int i) +static inline void cpuidle_free_state_kobj(struct cpuidle_device *device, + int count) { - kobject_put(&device->kobjs[i]->kobj); - wait_for_completion(&device->kobjs[i]->kobj_unregister); - kfree(device->kobjs[i]); - device->kobjs[i] = NULL; + int i; + + for (i = 0; i < count; i++) { + kobject_put(&device->kobjs[i]->kobj); + wait_for_completion(&device->kobjs[i]->kobj_unregister); + kfree(device->kobjs[i]); + device->kobjs[i] = NULL; + } }
/** @@ -419,8 +424,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device) return 0;
error_state: - for (i = i - 1; i >= 0; i--) - cpuidle_free_state_kobj(device, i); + cpuidle_free_state_kobj(device, i); return ret; }
@@ -430,10 +434,7 @@ error_state: */ static void cpuidle_remove_state_sysfs(struct cpuidle_device *device) { - int i; - - for (i = 0; i < device->state_count; i++) - cpuidle_free_state_kobj(device, i); + cpuidle_free_state_kobj(device, device->state_count); }
#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
Loop for states is currently present on callers side and so is replicated at several places. It would be better to move that inside cpuidle_free_state_kobj() instead.
This patch does it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
drivers/cpuidle/sysfs.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index e918b6d..ade31a9 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -378,12 +378,17 @@ static struct kobj_type ktype_state_cpuidle = { .release = cpuidle_state_sysfs_release, }; -static inline void cpuidle_free_state_kobj(struct cpuidle_device *device, int i) +static inline void cpuidle_free_state_kobj(struct cpuidle_device *device,
int count){
- kobject_put(&device->kobjs[i]->kobj);
- wait_for_completion(&device->kobjs[i]->kobj_unregister);
- kfree(device->kobjs[i]);
- device->kobjs[i] = NULL;
- int i;
- for (i = 0; i < count; i++) {
kobject_put(&device->kobjs[i]->kobj);wait_for_completion(&device->kobjs[i]->kobj_unregister);kfree(device->kobjs[i]);device->kobjs[i] = NULL;- }
} /** @@ -419,8 +424,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device) return 0; error_state:
- for (i = i - 1; i >= 0; i--)
cpuidle_free_state_kobj(device, i);
- cpuidle_free_state_kobj(device, i); return ret;
} @@ -430,10 +434,7 @@ error_state: */ static void cpuidle_remove_state_sysfs(struct cpuidle_device *device) {
- int i;
- for (i = 0; i < device->state_count; i++)
cpuidle_free_state_kobj(device, i);
- cpuidle_free_state_kobj(device, device->state_count);
} #ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
We always need to allocate struct cpuidle_device_kobj for all CPUs and so there is no real need to have a pointer to it inside struct cpuidle_device.
This patch makes a object instance of struct cpuidle_device_kobj inside struct cpuidle_device instead of a pointer.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/sysfs.c | 24 +++++------------------- include/linux/cpuidle.h | 11 +++++++++-- 2 files changed, 14 insertions(+), 21 deletions(-)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index ade31a9..db0aac3 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -172,12 +172,6 @@ struct cpuidle_attr {
#define attr_to_cpuidleattr(a) container_of(a, struct cpuidle_attr, attr)
-struct cpuidle_device_kobj { - struct cpuidle_device *dev; - struct completion kobj_unregister; - struct kobject kobj; -}; - static inline struct cpuidle_device *to_cpuidle_device(struct kobject *kobj) { struct cpuidle_device_kobj *kdev = @@ -399,7 +393,7 @@ static int cpuidle_add_state_sysfs(struct cpuidle_device *device) { int i, ret = -ENOMEM; struct cpuidle_state_kobj *kobj; - struct cpuidle_device_kobj *kdev = device->kobj_dev; + struct cpuidle_device_kobj *kdev = &device->kobj_dev; struct cpuidle_driver *drv = cpuidle_get_cpu_driver(device);
/* state statistics */ @@ -525,7 +519,7 @@ static struct kobj_type ktype_driver_cpuidle = { static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev) { struct cpuidle_driver_kobj *kdrv; - struct cpuidle_device_kobj *kdev = dev->kobj_dev; + struct cpuidle_device_kobj *kdev = &dev->kobj_dev; struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev); int ret;
@@ -606,24 +600,17 @@ void cpuidle_remove_device_sysfs(struct cpuidle_device *device) */ int cpuidle_add_sysfs(struct cpuidle_device *dev) { - struct cpuidle_device_kobj *kdev; + struct cpuidle_device_kobj *kdev = &dev->kobj_dev; struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu); int error;
- kdev = kzalloc(sizeof(*kdev), GFP_KERNEL); - if (!kdev) - return -ENOMEM; kdev->dev = dev; - dev->kobj_dev = kdev; - init_completion(&kdev->kobj_unregister);
error = kobject_init_and_add(&kdev->kobj, &ktype_cpuidle, &cpu_dev->kobj, "cpuidle"); - if (error) { - kfree(kdev); + if (error) return error; - }
kobject_uevent(&kdev->kobj, KOBJ_ADD);
@@ -636,9 +623,8 @@ int cpuidle_add_sysfs(struct cpuidle_device *dev) */ void cpuidle_remove_sysfs(struct cpuidle_device *dev) { - struct cpuidle_device_kobj *kdev = dev->kobj_dev; + struct cpuidle_device_kobj *kdev = &dev->kobj_dev;
kobject_put(&kdev->kobj); wait_for_completion(&kdev->kobj_unregister); - kfree(kdev); } diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index c082425..1fc5cb5 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -11,6 +11,8 @@ #ifndef _LINUX_CPUIDLE_H #define _LINUX_CPUIDLE_H
+#include <linux/completion.h> +#include <linux/kobject.h> #include <linux/percpu.h> #include <linux/list.h> #include <linux/hrtimer.h> @@ -59,10 +61,15 @@ struct cpuidle_state {
#define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
-struct cpuidle_device_kobj; struct cpuidle_state_kobj; struct cpuidle_driver_kobj;
+struct cpuidle_device_kobj { + struct cpuidle_device *dev; + struct completion kobj_unregister; + struct kobject kobj; +}; + struct cpuidle_device { unsigned int registered:1; unsigned int enabled:1; @@ -73,7 +80,7 @@ struct cpuidle_device { struct cpuidle_state_usage states_usage[CPUIDLE_STATE_MAX]; struct cpuidle_state_kobj *kobjs[CPUIDLE_STATE_MAX]; struct cpuidle_driver_kobj *kobj_driver; - struct cpuidle_device_kobj *kobj_dev; + struct cpuidle_device_kobj kobj_dev; struct list_head device_list;
#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
We always need to allocate struct cpuidle_device_kobj for all CPUs and so there is no real need to have a pointer to it inside struct cpuidle_device.
This patch makes a object instance of struct cpuidle_device_kobj inside struct cpuidle_device instead of a pointer.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
nack, it was made in purpose for kobject_init_and_add.
see commit 728ce22b696f9f1404a74d7b2279a65933553a1b
On 26 September 2013 03:42, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
We always need to allocate struct cpuidle_device_kobj for all CPUs and so there is no real need to have a pointer to it inside struct cpuidle_device.
This patch makes a object instance of struct cpuidle_device_kobj inside struct cpuidle_device instead of a pointer.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
nack, it was made in purpose for kobject_init_and_add.
see commit 728ce22b696f9f1404a74d7b2279a65933553a1b
Ahh.. sorry for missing that one :(
Now that I understand why it was introduced, I am thinking if we can make hotplug path a bit fast? By not freeing sysfs stuff and only hiding it for time being? And so we wouldn't be required to do unnecessary initialisations while coming back?
Would that be worth it?
On 09/26/2013 08:05 AM, Viresh Kumar wrote:
On 26 September 2013 03:42, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
We always need to allocate struct cpuidle_device_kobj for all CPUs and so there is no real need to have a pointer to it inside struct cpuidle_device.
This patch makes a object instance of struct cpuidle_device_kobj inside struct cpuidle_device instead of a pointer.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
nack, it was made in purpose for kobject_init_and_add.
see commit 728ce22b696f9f1404a74d7b2279a65933553a1b
Ahh.. sorry for missing that one :(
Now that I understand why it was introduced, I am thinking if we can make hotplug path a bit fast? By not freeing sysfs stuff and only hiding it for time being? And so we wouldn't be required to do unnecessary initialisations while coming back?
Would that be worth it?
Yes if it possible.
For CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, struct cpuidle_device always needs to allocate struct cpuidle_driver_kobj for all CPUs and so there is no real need to have a pointer to it inside struct cpuidle_device.
This patch makes a object instance of struct cpuidle_driver_kobj inside struct cpuidle_device instead of a pointer.
It also makes kobj_driver dependent on CONFIG_CPU_IDLE_MULTIPLE_DRIVERS.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/sysfs.c | 20 +++----------------- include/linux/cpuidle.h | 11 +++++++++-- 2 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index db0aac3..3386d64 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -439,12 +439,6 @@ static void cpuidle_remove_state_sysfs(struct cpuidle_device *device) static struct cpuidle_driver_attr attr_driver_##_name = \ __ATTR(_name, 0644, show, NULL)
-struct cpuidle_driver_kobj { - struct cpuidle_driver *drv; - struct completion kobj_unregister; - struct kobject kobj; -}; - struct cpuidle_driver_attr { struct attribute attr; ssize_t (*show)(struct cpuidle_driver *, char *); @@ -518,27 +512,20 @@ static struct kobj_type ktype_driver_cpuidle = { */ static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev) { - struct cpuidle_driver_kobj *kdrv; + struct cpuidle_driver_kobj *kdrv = &dev->kobj_driver; struct cpuidle_device_kobj *kdev = &dev->kobj_dev; struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev); int ret;
- kdrv = kzalloc(sizeof(*kdrv), GFP_KERNEL); - if (!kdrv) - return -ENOMEM; - kdrv->drv = drv; init_completion(&kdrv->kobj_unregister);
ret = kobject_init_and_add(&kdrv->kobj, &ktype_driver_cpuidle, &kdev->kobj, "driver"); - if (ret) { - kfree(kdrv); + if (ret) return ret; - }
kobject_uevent(&kdrv->kobj, KOBJ_ADD); - dev->kobj_driver = kdrv;
return ret; } @@ -549,10 +536,9 @@ static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev) */ static void cpuidle_remove_driver_sysfs(struct cpuidle_device *dev) { - struct cpuidle_driver_kobj *kdrv = dev->kobj_driver; + struct cpuidle_driver_kobj *kdrv = &dev->kobj_driver; kobject_put(&kdrv->kobj); wait_for_completion(&kdrv->kobj_unregister); - kfree(kdrv); } #else static inline int cpuidle_add_driver_sysfs(struct cpuidle_device *dev) diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 1fc5cb5..0f0da17 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -62,7 +62,6 @@ struct cpuidle_state { #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
struct cpuidle_state_kobj; -struct cpuidle_driver_kobj;
struct cpuidle_device_kobj { struct cpuidle_device *dev; @@ -70,6 +69,12 @@ struct cpuidle_device_kobj { struct kobject kobj; };
+struct cpuidle_driver_kobj { + struct cpuidle_driver *drv; + struct completion kobj_unregister; + struct kobject kobj; +}; + struct cpuidle_device { unsigned int registered:1; unsigned int enabled:1; @@ -79,7 +84,9 @@ struct cpuidle_device { int state_count; struct cpuidle_state_usage states_usage[CPUIDLE_STATE_MAX]; struct cpuidle_state_kobj *kobjs[CPUIDLE_STATE_MAX]; - struct cpuidle_driver_kobj *kobj_driver; +#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS + struct cpuidle_driver_kobj kobj_driver; +#endif struct cpuidle_device_kobj kobj_dev; struct list_head device_list;
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
For CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, struct cpuidle_device always needs to allocate struct cpuidle_driver_kobj for all CPUs and so there is no real need to have a pointer to it inside struct cpuidle_device.
This patch makes a object instance of struct cpuidle_driver_kobj inside struct cpuidle_device instead of a pointer.
It also makes kobj_driver dependent on CONFIG_CPU_IDLE_MULTIPLE_DRIVERS.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Comment similar to patch 16/21. If you try to offline/online a cpu, that should lead to a kernel warning.
drivers/cpuidle/sysfs.c | 20 +++----------------- include/linux/cpuidle.h | 11 +++++++++-- 2 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index db0aac3..3386d64 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -439,12 +439,6 @@ static void cpuidle_remove_state_sysfs(struct cpuidle_device *device) static struct cpuidle_driver_attr attr_driver_##_name = \ __ATTR(_name, 0644, show, NULL) -struct cpuidle_driver_kobj {
- struct cpuidle_driver *drv;
- struct completion kobj_unregister;
- struct kobject kobj;
-};
struct cpuidle_driver_attr { struct attribute attr; ssize_t (*show)(struct cpuidle_driver *, char *); @@ -518,27 +512,20 @@ static struct kobj_type ktype_driver_cpuidle = { */ static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev) {
- struct cpuidle_driver_kobj *kdrv;
- struct cpuidle_driver_kobj *kdrv = &dev->kobj_driver; struct cpuidle_device_kobj *kdev = &dev->kobj_dev; struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev); int ret;
- kdrv = kzalloc(sizeof(*kdrv), GFP_KERNEL);
- if (!kdrv)
return -ENOMEM;- kdrv->drv = drv; init_completion(&kdrv->kobj_unregister);
ret = kobject_init_and_add(&kdrv->kobj, &ktype_driver_cpuidle, &kdev->kobj, "driver");
- if (ret) {
kfree(kdrv);
- if (ret) return ret;
- }
kobject_uevent(&kdrv->kobj, KOBJ_ADD);
- dev->kobj_driver = kdrv;
return ret; } @@ -549,10 +536,9 @@ static int cpuidle_add_driver_sysfs(struct cpuidle_device *dev) */ static void cpuidle_remove_driver_sysfs(struct cpuidle_device *dev) {
- struct cpuidle_driver_kobj *kdrv = dev->kobj_driver;
- struct cpuidle_driver_kobj *kdrv = &dev->kobj_driver; kobject_put(&kdrv->kobj); wait_for_completion(&kdrv->kobj_unregister);
- kfree(kdrv);
} #else static inline int cpuidle_add_driver_sysfs(struct cpuidle_device *dev) diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 1fc5cb5..0f0da17 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -62,7 +62,6 @@ struct cpuidle_state { #define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000) struct cpuidle_state_kobj; -struct cpuidle_driver_kobj; struct cpuidle_device_kobj { struct cpuidle_device *dev; @@ -70,6 +69,12 @@ struct cpuidle_device_kobj { struct kobject kobj; }; +struct cpuidle_driver_kobj {
- struct cpuidle_driver *drv;
- struct completion kobj_unregister;
- struct kobject kobj;
+};
struct cpuidle_device { unsigned int registered:1; unsigned int enabled:1; @@ -79,7 +84,9 @@ struct cpuidle_device { int state_count; struct cpuidle_state_usage states_usage[CPUIDLE_STATE_MAX]; struct cpuidle_state_kobj *kobjs[CPUIDLE_STATE_MAX];
- struct cpuidle_driver_kobj *kobj_driver;
+#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS
- struct cpuidle_driver_kobj kobj_driver;
+#endif struct cpuidle_device_kobj kobj_dev; struct list_head device_list;
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 41 ----------------------------------------- drivers/cpuidle/driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 41 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 43d5836..bf80236 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -226,45 +226,6 @@ void cpuidle_resume(void) mutex_unlock(&cpuidle_lock); }
-#ifdef CONFIG_ARCH_HAS_CPU_RELAX -static int poll_idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index) -{ - ktime_t t1, t2; - s64 diff; - - t1 = ktime_get(); - local_irq_enable(); - while (!need_resched()) - cpu_relax(); - - t2 = ktime_get(); - diff = ktime_to_us(ktime_sub(t2, t1)); - if (diff > INT_MAX) - diff = INT_MAX; - - dev->last_residency = (int) diff; - - return index; -} - -static void poll_idle_init(struct cpuidle_driver *drv) -{ - struct cpuidle_state *state = &drv->states[0]; - - snprintf(state->name, CPUIDLE_NAME_LEN, "POLL"); - snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE"); - state->exit_latency = 0; - state->target_residency = 0; - state->power_usage = -1; - state->flags = 0; - state->enter = poll_idle; - state->disabled = false; -} -#else -static void poll_idle_init(struct cpuidle_driver *drv) {} -#endif /* CONFIG_ARCH_HAS_CPU_RELAX */ - /** * cpuidle_enable_device - enables idle PM for a CPU * @dev: the CPU @@ -294,8 +255,6 @@ int cpuidle_enable_device(struct cpuidle_device *dev) if (!dev->state_count) dev->state_count = drv->state_count;
- poll_idle_init(drv); - ret = cpuidle_add_device_sysfs(dev); if (ret) return ret; diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 7b2510a..a4a93b4 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -10,6 +10,7 @@
#include <linux/mutex.h> #include <linux/module.h> +#include <linux/sched.h> #include <linux/cpuidle.h> #include <linux/cpumask.h> #include <linux/clockchips.h> @@ -179,6 +180,45 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) } }
+#ifdef CONFIG_ARCH_HAS_CPU_RELAX +static int poll_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + ktime_t t1, t2; + s64 diff; + + t1 = ktime_get(); + local_irq_enable(); + while (!need_resched()) + cpu_relax(); + + t2 = ktime_get(); + diff = ktime_to_us(ktime_sub(t2, t1)); + if (diff > INT_MAX) + diff = INT_MAX; + + dev->last_residency = (int) diff; + + return index; +} + +static void poll_idle_init(struct cpuidle_driver *drv) +{ + struct cpuidle_state *state = &drv->states[0]; + + snprintf(state->name, CPUIDLE_NAME_LEN, "POLL"); + snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE"); + state->exit_latency = 0; + state->target_residency = 0; + state->power_usage = -1; + state->flags = 0; + state->enter = poll_idle; + state->disabled = false; +} +#else +static void poll_idle_init(struct cpuidle_driver *drv) {} +#endif /* !CONFIG_ARCH_HAS_CPU_RELAX */ + /** * __cpuidle_register_driver: register the driver * @drv: a valid pointer to a struct cpuidle_driver @@ -212,6 +252,8 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv) on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer, (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1);
+ poll_idle_init(drv); + return 0; }
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
The optimization sounds good but IMHO if we can move this state out of the cpuidle common framework that would be nicer.
The poll_idle is only applicable for x86 (acpi_driver and intel_idle), hence I suggest we move this state to these drivers, that will cleanup the framework code and will remove index shift macro CPUIDLE_DRIVER_STATE_START which IMHO is weid and prone-to-error.
drivers/cpuidle/cpuidle.c | 41 ----------------------------------------- drivers/cpuidle/driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 41 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 43d5836..bf80236 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -226,45 +226,6 @@ void cpuidle_resume(void) mutex_unlock(&cpuidle_lock); } -#ifdef CONFIG_ARCH_HAS_CPU_RELAX -static int poll_idle(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int index)-{
- ktime_t t1, t2;
- s64 diff;
- t1 = ktime_get();
- local_irq_enable();
- while (!need_resched())
cpu_relax();- t2 = ktime_get();
- diff = ktime_to_us(ktime_sub(t2, t1));
- if (diff > INT_MAX)
diff = INT_MAX;- dev->last_residency = (int) diff;
- return index;
-}
-static void poll_idle_init(struct cpuidle_driver *drv) -{
- struct cpuidle_state *state = &drv->states[0];
- snprintf(state->name, CPUIDLE_NAME_LEN, "POLL");
- snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE");
- state->exit_latency = 0;
- state->target_residency = 0;
- state->power_usage = -1;
- state->flags = 0;
- state->enter = poll_idle;
- state->disabled = false;
-} -#else -static void poll_idle_init(struct cpuidle_driver *drv) {} -#endif /* CONFIG_ARCH_HAS_CPU_RELAX */
/**
- cpuidle_enable_device - enables idle PM for a CPU
- @dev: the CPU
@@ -294,8 +255,6 @@ int cpuidle_enable_device(struct cpuidle_device *dev) if (!dev->state_count) dev->state_count = drv->state_count;
- poll_idle_init(drv);
- ret = cpuidle_add_device_sysfs(dev); if (ret) return ret;
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 7b2510a..a4a93b4 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -10,6 +10,7 @@ #include <linux/mutex.h> #include <linux/module.h> +#include <linux/sched.h> #include <linux/cpuidle.h> #include <linux/cpumask.h> #include <linux/clockchips.h> @@ -179,6 +180,45 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) } } +#ifdef CONFIG_ARCH_HAS_CPU_RELAX +static int poll_idle(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int index)+{
- ktime_t t1, t2;
- s64 diff;
- t1 = ktime_get();
- local_irq_enable();
- while (!need_resched())
cpu_relax();- t2 = ktime_get();
- diff = ktime_to_us(ktime_sub(t2, t1));
- if (diff > INT_MAX)
diff = INT_MAX;- dev->last_residency = (int) diff;
- return index;
+}
+static void poll_idle_init(struct cpuidle_driver *drv) +{
- struct cpuidle_state *state = &drv->states[0];
- snprintf(state->name, CPUIDLE_NAME_LEN, "POLL");
- snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE");
- state->exit_latency = 0;
- state->target_residency = 0;
- state->power_usage = -1;
- state->flags = 0;
- state->enter = poll_idle;
- state->disabled = false;
+} +#else +static void poll_idle_init(struct cpuidle_driver *drv) {} +#endif /* !CONFIG_ARCH_HAS_CPU_RELAX */
/**
- __cpuidle_register_driver: register the driver
- @drv: a valid pointer to a struct cpuidle_driver
@@ -212,6 +252,8 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv) on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer, (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1);
- poll_idle_init(drv);
- return 0;
}
On 26 September 2013 03:52, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
This deserved a log, sorry for missing that :(
The optimization sounds good but IMHO if we can move this state out of the cpuidle common framework that would be nicer.
The poll_idle is only applicable for x86 (acpi_driver and intel_idle), hence I suggest we move this state to these drivers, that will cleanup the framework code and will remove index shift macro CPUIDLE_DRIVER_STATE_START which IMHO is weid and prone-to-error.
Lets see what X86 folks have to say about it and then we can do it.. Btw, wouldn't that add some code duplication in those two drivers?
On 09/26/2013 08:09 AM, Viresh Kumar wrote:
On 26 September 2013 03:52, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
This deserved a log, sorry for missing that :(
The optimization sounds good but IMHO if we can move this state out of the cpuidle common framework that would be nicer.
The poll_idle is only applicable for x86 (acpi_driver and intel_idle), hence I suggest we move this state to these drivers, that will cleanup the framework code and will remove index shift macro CPUIDLE_DRIVER_STATE_START which IMHO is weid and prone-to-error.
Lets see what X86 folks have to say about it and then we can do it.. Btw, wouldn't that add some code duplication in those two drivers?
Yes, certainly and that will impact also the menu select governor function:
...
/* * We want to default to C1 (hlt), not to busy polling * unless the timer is happening really really soon. */ if (data->expected_us > 5 && !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
/* * Find the idle state with the lowest power while satisfying * our constraints. */ for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { struct cpuidle_state *s = &drv->states[i]; struct cpuidle_state_usage *su = &dev->states_usage[i];
if (s->disabled || su->disable) continue; if (s->target_residency > data->predicted_us) continue; if (s->exit_latency > latency_req) continue; if (s->exit_latency * multiplier > data->predicted_us) continue;
data->last_state_idx = i; data->exit_us = s->exit_latency; }
....
On 26 September 2013 13:58, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Yes, certainly and that will impact also the menu select governor function:
...
/* * We want to default to C1 (hlt), not to busy polling * unless the timer is happening really really soon. */ if (data->expected_us > 5 && !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START; /* * Find the idle state with the lowest power while satisfying * our constraints. */ for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { struct cpuidle_state *s = &drv->states[i]; struct cpuidle_state_usage *su = &dev->states_usage[i]; if (s->disabled || su->disable) continue; if (s->target_residency > data->predicted_us) continue; if (s->exit_latency > latency_req) continue; if (s->exit_latency * multiplier > data->predicted_us) continue; data->last_state_idx = i; data->exit_us = s->exit_latency; }
Hmm.. For now I will repost this patch as is and then you can go ahead for this bigger change.. I wouldn't be able to do this change now, as I would be rushing for my 2 weeks vacations :)
If this patch looked okay to you, can you please Ack it ?
On 10/03/2013 12:33 PM, Viresh Kumar wrote:
On 26 September 2013 13:58, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Yes, certainly and that will impact also the menu select governor function:
...
/* * We want to default to C1 (hlt), not to busy polling * unless the timer is happening really really soon. */ if (data->expected_us > 5 && !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START; /* * Find the idle state with the lowest power while satisfying * our constraints. */ for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { struct cpuidle_state *s = &drv->states[i]; struct cpuidle_state_usage *su = &dev->states_usage[i]; if (s->disabled || su->disable) continue; if (s->target_residency > data->predicted_us) continue; if (s->exit_latency > latency_req) continue; if (s->exit_latency * multiplier > data->predicted_us) continue; data->last_state_idx = i; data->exit_us = s->exit_latency; }Hmm.. For now I will repost this patch as is and then you can go ahead for this bigger change.. I wouldn't be able to do this change now, as I would be rushing for my 2 weeks vacations :)
If this patch looked okay to you, can you please Ack it ?
Acked-by: Daniel Lezcano daniel.lezcano@linaro.org
Currently we have multiple definitions of few routines based on following config option: CONFIG_CPU_IDLE_MULTIPLE_DRIVERS.
These are present to save space by not creating per-cpu variable for platforms which need only one cpuidle driver to be registered for all CPUs.
But this setup has a problem. For ARM multi-platform kernel use case this option will get enabled and so we will have per-cpu variables even for platforms that don't need it.
The bigger problem is two separate code paths for such platforms for single & multi platform kernels. Which doesn't sound good.
A better way of solving this problem would be to create cpuidle driver's list that can be used to manage all information we need. Then we don't really have to write any special code for handling platforms with CONFIG_CPU_IDLE_MULTIPLE_DRIVERS option set.
This patch does it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/driver.c | 106 ++++++++++++----------------------------------- include/linux/cpuidle.h | 1 + 2 files changed, 27 insertions(+), 80 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index a4a93b4..320b4ec 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -18,10 +18,19 @@ #include "cpuidle.h"
DEFINE_SPINLOCK(cpuidle_driver_lock); +static LIST_HEAD(cpuidle_detected_drivers);
-#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS +static inline struct cpuidle_driver * +__cpuidle_get_driver(const struct cpumask *cpumask) +{ + struct cpuidle_driver *drv; + + list_for_each_entry(drv, &cpuidle_detected_drivers, driver_list) + if (cpumask_intersects(drv->cpumask, cpumask)) + return drv;
-static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers); + return NULL; +}
/** * __cpuidle_get_cpu_driver - return the cpuidle driver tied to a CPU. @@ -32,103 +41,39 @@ static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers); */ static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) { - return per_cpu(cpuidle_drivers, cpu); + return __cpuidle_get_driver(cpumask_of(cpu)); }
/** - * __cpuidle_unset_driver - unset per CPU driver variables. + * __cpuidle_add_driver - adds a cpuidle driver to list. * @drv: a valid pointer to a struct cpuidle_driver * - * For each CPU in the driver's CPU mask, unset the registered driver per CPU - * variable. If @drv is different from the registered driver, the corresponding - * variable is not cleared. - */ -static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) -{ - int cpu; - - for_each_cpu(cpu, drv->cpumask) { - - if (drv != __cpuidle_get_cpu_driver(cpu)) - continue; - - per_cpu(cpuidle_drivers, cpu) = NULL; - } -} - -/** - * __cpuidle_set_driver - set per CPU driver variables for the given driver. - * @drv: a valid pointer to a struct cpuidle_driver - * - * For each CPU in the driver's cpumask, unset the registered driver per CPU - * to @drv. + * Adds cpuidle driver to cpuidle_detected_drivers list if no driver is already + * registered for any CPUs present in drv->cpumask. * * Returns 0 on success, -EBUSY if the CPUs have driver(s) already. */ -static inline int __cpuidle_set_driver(struct cpuidle_driver *drv) -{ - int cpu; - - for_each_cpu(cpu, drv->cpumask) { - - if (__cpuidle_get_cpu_driver(cpu)) { - __cpuidle_unset_driver(drv); - return -EBUSY; - } - - per_cpu(cpuidle_drivers, cpu) = drv; - } - - return 0; -} - -#else - -static struct cpuidle_driver *cpuidle_curr_driver; - -/** - * __cpuidle_get_cpu_driver - return the global cpuidle driver pointer. - * @cpu: ignored without the multiple driver support - * - * Return a pointer to a struct cpuidle_driver object or NULL if no driver was - * previously registered. - */ -static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) -{ - return cpuidle_curr_driver; -} - -/** - * __cpuidle_set_driver - assign the global cpuidle driver variable. - * @drv: pointer to a struct cpuidle_driver object - * - * Returns 0 on success, -EBUSY if the driver is already registered. - */ -static inline int __cpuidle_set_driver(struct cpuidle_driver *drv) +static inline int __cpuidle_add_driver(struct cpuidle_driver *drv) { - if (cpuidle_curr_driver) + if (__cpuidle_get_driver(drv->cpumask)) return -EBUSY;
- cpuidle_curr_driver = drv; + list_add(&drv->driver_list, &cpuidle_detected_drivers);
return 0; }
/** - * __cpuidle_unset_driver - unset the global cpuidle driver variable. - * @drv: a pointer to a struct cpuidle_driver + * __cpuidle_remove_driver - remove cpuidle driver from list. + * @drv: a valid pointer to a struct cpuidle_driver * - * Reset the global cpuidle variable to NULL. If @drv does not match the - * registered driver, do nothing. + * Removes cpuidle driver from cpuidle_detected_drivers list. */ -static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) +static inline void __cpuidle_remove_driver(struct cpuidle_driver *drv) { - if (drv == cpuidle_curr_driver) - cpuidle_curr_driver = NULL; + list_del(&drv->driver_list); }
-#endif - /** * cpuidle_setup_broadcast_timer - enable/disable the broadcast timer * @arg: a void pointer used to match the SMP cross call API @@ -158,6 +103,7 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) int i;
drv->refcnt = 0; + INIT_LIST_HEAD(&drv->driver_list);
/* * Use all possible CPUs as the default, because if the kernel boots @@ -244,7 +190,7 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv)
__cpuidle_driver_init(drv);
- ret = __cpuidle_set_driver(drv); + ret = __cpuidle_add_driver(drv); if (ret) return ret;
@@ -277,7 +223,7 @@ static void __cpuidle_unregister_driver(struct cpuidle_driver *drv) (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1); }
- __cpuidle_unset_driver(drv); + __cpuidle_remove_driver(drv); }
/** diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 0f0da17..81b74d2 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -129,6 +129,7 @@ struct cpuidle_driver {
/* the driver handles the cpus in cpumask */ struct cpumask *cpumask; + struct list_head driver_list; };
#ifdef CONFIG_CPU_IDLE
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
Currently we have multiple definitions of few routines based on following config option: CONFIG_CPU_IDLE_MULTIPLE_DRIVERS.
These are present to save space by not creating per-cpu variable for platforms which need only one cpuidle driver to be registered for all CPUs.
But this setup has a problem. For ARM multi-platform kernel use case this option will get enabled and so we will have per-cpu variables even for platforms that don't need it.
The bigger problem is two separate code paths for such platforms for single & multi platform kernels. Which doesn't sound good.
A better way of solving this problem would be to create cpuidle driver's list that can be used to manage all information we need. Then we don't really have to write any special code for handling platforms with CONFIG_CPU_IDLE_MULTIPLE_DRIVERS option set.
This patch does it.
If you introduce a list, you will have to introduce a lock to protect it. This lock will be in the fast path cpuidle_idle_call with the get_driver function and conforming to the comment: "NOTE: no locks or semaphores should be used here".
A lock has been introduced in this function already and the system hangs with 1024 cpus.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/driver.c | 106 ++++++++++++----------------------------------- include/linux/cpuidle.h | 1 + 2 files changed, 27 insertions(+), 80 deletions(-)
diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index a4a93b4..320b4ec 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -18,10 +18,19 @@ #include "cpuidle.h" DEFINE_SPINLOCK(cpuidle_driver_lock); +static LIST_HEAD(cpuidle_detected_drivers); -#ifdef CONFIG_CPU_IDLE_MULTIPLE_DRIVERS +static inline struct cpuidle_driver * +__cpuidle_get_driver(const struct cpumask *cpumask) +{
- struct cpuidle_driver *drv;
- list_for_each_entry(drv, &cpuidle_detected_drivers, driver_list)
if (cpumask_intersects(drv->cpumask, cpumask))return drv;-static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers);
- return NULL;
+} /**
- __cpuidle_get_cpu_driver - return the cpuidle driver tied to a CPU.
@@ -32,103 +41,39 @@ static DEFINE_PER_CPU(struct cpuidle_driver *, cpuidle_drivers); */ static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) {
- return per_cpu(cpuidle_drivers, cpu);
- return __cpuidle_get_driver(cpumask_of(cpu));
} /**
- __cpuidle_unset_driver - unset per CPU driver variables.
- __cpuidle_add_driver - adds a cpuidle driver to list.
- @drv: a valid pointer to a struct cpuidle_driver
- For each CPU in the driver's CPU mask, unset the registered driver per CPU
- variable. If @drv is different from the registered driver, the corresponding
- variable is not cleared.
- */
-static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) -{
- int cpu;
- for_each_cpu(cpu, drv->cpumask) {
if (drv != __cpuidle_get_cpu_driver(cpu))continue;per_cpu(cpuidle_drivers, cpu) = NULL;- }
-}
-/**
- __cpuidle_set_driver - set per CPU driver variables for the given driver.
- @drv: a valid pointer to a struct cpuidle_driver
- For each CPU in the driver's cpumask, unset the registered driver per CPU
- to @drv.
- Adds cpuidle driver to cpuidle_detected_drivers list if no driver is already
*/
- registered for any CPUs present in drv->cpumask.
- Returns 0 on success, -EBUSY if the CPUs have driver(s) already.
-static inline int __cpuidle_set_driver(struct cpuidle_driver *drv) -{
- int cpu;
- for_each_cpu(cpu, drv->cpumask) {
if (__cpuidle_get_cpu_driver(cpu)) {__cpuidle_unset_driver(drv);return -EBUSY;}per_cpu(cpuidle_drivers, cpu) = drv;- }
- return 0;
-}
-#else
-static struct cpuidle_driver *cpuidle_curr_driver;
-/**
- __cpuidle_get_cpu_driver - return the global cpuidle driver pointer.
- @cpu: ignored without the multiple driver support
- Return a pointer to a struct cpuidle_driver object or NULL if no driver was
- previously registered.
- */
-static inline struct cpuidle_driver *__cpuidle_get_cpu_driver(int cpu) -{
- return cpuidle_curr_driver;
-}
-/**
- __cpuidle_set_driver - assign the global cpuidle driver variable.
- @drv: pointer to a struct cpuidle_driver object
- Returns 0 on success, -EBUSY if the driver is already registered.
- */
-static inline int __cpuidle_set_driver(struct cpuidle_driver *drv) +static inline int __cpuidle_add_driver(struct cpuidle_driver *drv) {
- if (cpuidle_curr_driver)
- if (__cpuidle_get_driver(drv->cpumask)) return -EBUSY;
- cpuidle_curr_driver = drv;
- list_add(&drv->driver_list, &cpuidle_detected_drivers);
return 0; } /**
- __cpuidle_unset_driver - unset the global cpuidle driver variable.
- @drv: a pointer to a struct cpuidle_driver
- __cpuidle_remove_driver - remove cpuidle driver from list.
- @drv: a valid pointer to a struct cpuidle_driver
- Reset the global cpuidle variable to NULL. If @drv does not match the
- registered driver, do nothing.
*/
- Removes cpuidle driver from cpuidle_detected_drivers list.
-static inline void __cpuidle_unset_driver(struct cpuidle_driver *drv) +static inline void __cpuidle_remove_driver(struct cpuidle_driver *drv) {
- if (drv == cpuidle_curr_driver)
cpuidle_curr_driver = NULL;
- list_del(&drv->driver_list);
} -#endif
/**
- cpuidle_setup_broadcast_timer - enable/disable the broadcast timer
- @arg: a void pointer used to match the SMP cross call API
@@ -158,6 +103,7 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) int i; drv->refcnt = 0;
- INIT_LIST_HEAD(&drv->driver_list);
/* * Use all possible CPUs as the default, because if the kernel boots @@ -244,7 +190,7 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv) __cpuidle_driver_init(drv);
- ret = __cpuidle_set_driver(drv);
- ret = __cpuidle_add_driver(drv); if (ret) return ret;
@@ -277,7 +223,7 @@ static void __cpuidle_unregister_driver(struct cpuidle_driver *drv) (void *)CLOCK_EVT_NOTIFY_BROADCAST_OFF, 1); }
- __cpuidle_unset_driver(drv);
- __cpuidle_remove_driver(drv);
} /** diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 0f0da17..81b74d2 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -129,6 +129,7 @@ struct cpuidle_driver { /* the driver handles the cpus in cpumask */ struct cpumask *cpumask;
- struct list_head driver_list;
}; #ifdef CONFIG_CPU_IDLE
On 26 September 2013 04:00, Daniel Lezcano daniel.lezcano@linaro.org wrote:
If you introduce a list, you will have to introduce a lock to protect it.
I missed it, should have added that :)
This lock will be in the fast path cpuidle_idle_call with the get_driver function and conforming to the comment: "NOTE: no locks or semaphores should be used here".
A lock has been introduced in this function already and the system hangs with 1024 cpus.
Hmm... I see.. I didn't knew about this expectation.. What about a rcu read/write lock? As far as I know its too lightweight... Can we have that in fast path?
On 09/26/2013 08:17 AM, Viresh Kumar wrote:
On 26 September 2013 04:00, Daniel Lezcano daniel.lezcano@linaro.org wrote:
If you introduce a list, you will have to introduce a lock to protect it.
I missed it, should have added that :)
This lock will be in the fast path cpuidle_idle_call with the get_driver function and conforming to the comment: "NOTE: no locks or semaphores should be used here".
A lock has been introduced in this function already and the system hangs with 1024 cpus.
Hmm... I see.. I didn't knew about this expectation.. What about a rcu read/write lock? As far as I know its too lightweight... Can we have that in fast path?
Nope, we can't use rcu in the idle path :)
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-February/083054.h...
On Thu, Sep 26, 2013 at 10:19:14AM +0200, Daniel Lezcano wrote:
On 09/26/2013 08:17 AM, Viresh Kumar wrote:
On 26 September 2013 04:00, Daniel Lezcano daniel.lezcano@linaro.org wrote:
If you introduce a list, you will have to introduce a lock to protect it.
I missed it, should have added that :)
This lock will be in the fast path cpuidle_idle_call with the get_driver function and conforming to the comment: "NOTE: no locks or semaphores should be used here".
A lock has been introduced in this function already and the system hangs with 1024 cpus.
Hmm... I see.. I didn't knew about this expectation.. What about a rcu read/write lock? As far as I know its too lightweight... Can we have that in fast path?
Nope, we can't use rcu in the idle path :)
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-February/083054.h...
But you should be able to use SRCU in the idle path, if that helps.
Thanx, Paul
On 09/28/2013 11:33 PM, Paul E. McKenney wrote:
On Thu, Sep 26, 2013 at 10:19:14AM +0200, Daniel Lezcano wrote:
On 09/26/2013 08:17 AM, Viresh Kumar wrote:
On 26 September 2013 04:00, Daniel Lezcano daniel.lezcano@linaro.org wrote:
If you introduce a list, you will have to introduce a lock to protect it.
I missed it, should have added that :)
This lock will be in the fast path cpuidle_idle_call with the get_driver function and conforming to the comment: "NOTE: no locks or semaphores should be used here".
A lock has been introduced in this function already and the system hangs with 1024 cpus.
Hmm... I see.. I didn't knew about this expectation.. What about a rcu read/write lock? As far as I know its too lightweight... Can we have that in fast path?
Nope, we can't use rcu in the idle path :)
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-February/083054.h...
But you should be able to use SRCU in the idle path, if that helps.
Interesting, thanks for the pointer.
-- Daniel
On 1 October 2013 00:07, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Interesting, thanks for the pointer.
So, should I keep this patch with SRCU?
On 10/03/2013 06:38 AM, Viresh Kumar wrote:
On 1 October 2013 00:07, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Interesting, thanks for the pointer.
So, should I keep this patch with SRCU?
IMHO, we should, for now, keep the code as it is and then focus on the lock/refcount for drivers in a separate series.
If entered_state == 0, we don't need to set dev->last_residency to 'diff' as we will be setting it to zero without using its new value.
And so move calculation of diff also inside the "if" statement.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/cpuidle.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index bf80236..cb81689 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -77,23 +77,22 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
struct cpuidle_state *target_state = &drv->states[index]; ktime_t time_start, time_end; - s64 diff;
time_start = ktime_get();
entered_state = target_state->enter(dev, drv, index);
- time_end = ktime_get(); + if (entered_state >= 0) { + s64 diff;
- local_irq_enable(); + time_end = ktime_get(); + diff = ktime_to_us(ktime_sub(time_end, time_start));
- diff = ktime_to_us(ktime_sub(time_end, time_start)); - if (diff > INT_MAX) - diff = INT_MAX; + if (diff > INT_MAX) + diff = INT_MAX;
- dev->last_residency = (int) diff; + dev->last_residency = (int) diff;
- if (entered_state >= 0) { /* Update cpuidle counters */ /* This can be moved to within driver enter routine * but that results in multiple copies of same code. @@ -104,6 +103,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv, dev->last_residency = 0; }
+ local_irq_enable(); + return entered_state; }
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
If entered_state == 0, we don't need to set dev->last_residency to 'diff' as we will be setting it to zero without using its new value.
I don't get it, can you elaborate. We can be a long time in this state (eg. if the prediction is false).
And so move calculation of diff also inside the "if" statement. Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/cpuidle.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index bf80236..cb81689 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -77,23 +77,22 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv, struct cpuidle_state *target_state = &drv->states[index]; ktime_t time_start, time_end;
- s64 diff;
time_start = ktime_get(); entered_state = target_state->enter(dev, drv, index);
- time_end = ktime_get();
- if (entered_state >= 0) {
s64 diff;
- local_irq_enable();
time_end = ktime_get();diff = ktime_to_us(ktime_sub(time_end, time_start));
- diff = ktime_to_us(ktime_sub(time_end, time_start));
- if (diff > INT_MAX)
diff = INT_MAX;
if (diff > INT_MAX)diff = INT_MAX;
- dev->last_residency = (int) diff;
dev->last_residency = (int) diff;
- if (entered_state >= 0) { /* Update cpuidle counters */ /* This can be moved to within driver enter routine
- but that results in multiple copies of same code.
@@ -104,6 +103,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv, dev->last_residency = 0; }
- local_irq_enable();
- return entered_state;
}
On 26 September 2013 04:08, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
If entered_state == 0, we don't need to set dev->last_residency to 'diff' as we will be setting it to zero without using its new value.
I don't get it, can you elaborate.
Sure.. We have following code in cpuidle_enter_state():
--------- diff = ktime_to_us(ktime_sub(time_end, time_start)); if (diff > INT_MAX) diff = INT_MAX;
dev->last_residency = (int) diff;
if (entered_state >= 0) { /* Update cpuidle counters */ /* This can be moved to within driver enter routine * but that results in multiple copies of same code. */ dev->states_usage[entered_state].time += dev->last_residency; dev->states_usage[entered_state].usage++; } else { dev->last_residency = 0; } -------
We are setting last_residency to 0 when (entered_state < 0) and aren't using the value of "diff". So, we can actually skip calculations of time_end, diff and last_residency when (entered_state < 0).. Makes sense?
We can be a long time in this state (eg. if the prediction is false).
Okay.. I didn't get it :)
On 09/26/2013 08:24 AM, Viresh Kumar wrote:
On 26 September 2013 04:08, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
If entered_state == 0, we don't need to set dev->last_residency to 'diff' as we will be setting it to zero without using its new value.
I don't get it, can you elaborate.
Sure.. We have following code in cpuidle_enter_state():
diff = ktime_to_us(ktime_sub(time_end, time_start)); if (diff > INT_MAX) diff = INT_MAX; dev->last_residency = (int) diff; if (entered_state >= 0) { /* Update cpuidle counters */ /* This can be moved to within driver enter routine * but that results in multiple copies of same code. */ dev->states_usage[entered_state].time += dev->last_residency; dev->states_usage[entered_state].usage++; } else { dev->last_residency = 0; }
We are setting last_residency to 0 when (entered_state < 0) and aren't using the value of "diff". So, we can actually skip calculations of time_end, diff and last_residency when (entered_state < 0).. Makes sense?
Yes I agree, but why are you saying "If entered_state == 0, we don't need to ..." ?
We can be a long time in this state (eg. if the prediction is false).
Okay.. I didn't get it :)
On 26 September 2013 13:55, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Yes I agree, but why are you saying "If entered_state == 0, we don't need to ..." ?
Ahh.. that's a mistake.. s/==/< :)
On 09/26/2013 10:28 AM, Viresh Kumar wrote:
On 26 September 2013 13:55, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Yes I agree, but why are you saying "If entered_state == 0, we don't need to ..." ?
Ahh.. that's a mistake.. s/==/< :)
Ah ok.
When I first read cpuidle_replace_governor()'s name I thought it will replace the governor (as per its name) but then found that it just returns the next best governor. And cpuidle_unregister_governor() actually replaces it.
We always replace current governor with the next best and this information is already present with cpuidle_replace_governor() and so we don't really need to send an additional argument for it.
Also, it makes sense to actually call cpuidle_switch_governor() from within cpuidle_replace_governor() instead.
Along with this ret_gov is now renamed as new_gov to better suit its purpose.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpuidle/governor.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c index ea2f8e7..deb6e50 100644 --- a/drivers/cpuidle/governor.c +++ b/drivers/cpuidle/governor.c @@ -98,26 +98,27 @@ int cpuidle_register_governor(struct cpuidle_governor *gov) }
/** - * cpuidle_replace_governor - find a replacement governor - * @exclude_rating: the rating that will be skipped while looking for - * new governor. + * cpuidle_replace_governor - replace governor with highest rating one + * + * Finds governor (excluding cpuidle_curr_governor) with highest rating and + * replaces cpuidle_curr_governor with it. */ -static struct cpuidle_governor *cpuidle_replace_governor(int exclude_rating) +static inline void cpuidle_replace_governor(void) { struct cpuidle_governor *gov; - struct cpuidle_governor *ret_gov = NULL; + struct cpuidle_governor *new_gov = NULL; unsigned int max_rating = 0;
list_for_each_entry(gov, &cpuidle_governors, governor_list) { - if (gov->rating == exclude_rating) + if (gov == cpuidle_curr_governor) continue; if (gov->rating > max_rating) { max_rating = gov->rating; - ret_gov = gov; + new_gov = gov; } }
- return ret_gov; + cpuidle_switch_governor(new_gov); }
/** @@ -130,11 +131,8 @@ void cpuidle_unregister_governor(struct cpuidle_governor *gov) return;
mutex_lock(&cpuidle_lock); - if (gov == cpuidle_curr_governor) { - struct cpuidle_governor *new_gov; - new_gov = cpuidle_replace_governor(gov->rating); - cpuidle_switch_governor(new_gov); - } + if (gov == cpuidle_curr_governor) + cpuidle_replace_governor(); list_del(&gov->governor_list); mutex_unlock(&cpuidle_lock); }
On 09/22/2013 03:21 AM, Viresh Kumar wrote:
When I first read cpuidle_replace_governor()'s name I thought it will replace the governor (as per its name) but then found that it just returns the next best governor. And cpuidle_unregister_governor() actually replaces it.
We always replace current governor with the next best and this information is already present with cpuidle_replace_governor() and so we don't really need to send an additional argument for it.
Also, it makes sense to actually call cpuidle_switch_governor() from within cpuidle_replace_governor() instead.
Along with this ret_gov is now renamed as new_gov to better suit its purpose.
Actually I am wondering if all this stuff is not over-engineered.
There are 2 governors, each of them suits for a specific kernel config, periodic tick or tickless system.
menu => tickless system periodic => periodic tick system
IMHO, all the code with rating checking and so do not really makes sense. Wouldn't make sense to remove this code ?
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpuidle/governor.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c index ea2f8e7..deb6e50 100644 --- a/drivers/cpuidle/governor.c +++ b/drivers/cpuidle/governor.c @@ -98,26 +98,27 @@ int cpuidle_register_governor(struct cpuidle_governor *gov) } /**
- cpuidle_replace_governor - find a replacement governor
- @exclude_rating: the rating that will be skipped while looking for
- new governor.
- cpuidle_replace_governor - replace governor with highest rating one
- Finds governor (excluding cpuidle_curr_governor) with highest rating and
*/
- replaces cpuidle_curr_governor with it.
-static struct cpuidle_governor *cpuidle_replace_governor(int exclude_rating) +static inline void cpuidle_replace_governor(void) { struct cpuidle_governor *gov;
- struct cpuidle_governor *ret_gov = NULL;
- struct cpuidle_governor *new_gov = NULL; unsigned int max_rating = 0;
list_for_each_entry(gov, &cpuidle_governors, governor_list) {
if (gov->rating == exclude_rating)
if (gov == cpuidle_curr_governor) continue;
ret_gov = gov;
new_gov = gov;
- return ret_gov;
- cpuidle_switch_governor(new_gov);
} /** @@ -130,11 +131,8 @@ void cpuidle_unregister_governor(struct cpuidle_governor *gov) return; mutex_lock(&cpuidle_lock);
- if (gov == cpuidle_curr_governor) {
struct cpuidle_governor *new_gov;new_gov = cpuidle_replace_governor(gov->rating);cpuidle_switch_governor(new_gov);- }
- if (gov == cpuidle_curr_governor)
cpuidle_replace_governor();}
On 26 September 2013 04:20, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Actually I am wondering if all this stuff is not over-engineered.
There are 2 governors, each of them suits for a specific kernel config, periodic tick or tickless system.
menu => tickless system periodic => periodic tick system
IMHO, all the code with rating checking and so do not really makes sense. Wouldn't make sense to remove this code ?
I am a newbie here, really can't think of all side effects of this :)
On 09/26/2013 08:37 AM, Viresh Kumar wrote:
On 26 September 2013 04:20, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Actually I am wondering if all this stuff is not over-engineered.
There are 2 governors, each of them suits for a specific kernel config, periodic tick or tickless system.
menu => tickless system periodic => periodic tick system
IMHO, all the code with rating checking and so do not really makes sense. Wouldn't make sense to remove this code ?
I am a newbie here, really can't think of all side effects of this :)
Rafael is pretty busy ATM but may be he can give his feedback on this later.
On 26 September 2013 13:50, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Rafael is pretty busy ATM but may be he can give his feedback on this later.
For now I will resend it and maybe later you can get it cleaned up even more.. Or maybe I will do it once I have better hold on cpuidle core :)
Can I have your Ack for now? (As discussed on IRC) :)
-- viresh
On 10/03/2013 12:36 PM, Viresh Kumar wrote:
On 26 September 2013 13:50, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Rafael is pretty busy ATM but may be he can give his feedback on this later.
For now I will resend it and maybe later you can get it cleaned up even more.. Or maybe I will do it once I have better hold on cpuidle core :)
Can I have your Ack for now? (As discussed on IRC) :)
Actually the functions cpuidle_unregister_governor and cpuidle_replace_governor are dead code since the governors are no longer modules (commit 137b944e100278d696826cf25c83014ac17473fe), so you can remove the code instead.
linaro-kernel@lists.linaro.org
-
Colin Cross -
Daniel Lezcano -
Hongbo Zhang -
Paul E. McKenney -
Viresh Kumar