cpuidle: move field disable from per-driver to per-cpu
PATCH 1/1 cpuidle: move field disable from per-driver to per-cpu
From: ShuoX Liu <[email protected]>
Andrew J.Schorr raises a question. When he changes the disable setting on a single CPU, it affects all the other CPUs. Basically, currently, the disable field is per-driver instead of per-cpu. All the C states of the same driver are shared by all CPU in the same machine.
Below patch changes field disable to per-cpu, so we could set this separately for each cpu.
--- drivers/cpuidle/cpuidle.c | 1 - drivers/cpuidle/governors/menu.c | 5 +++-- drivers/cpuidle/sysfs.c | 21 ++++++++++++--------- include/linux/cpuidle.h | 2 +- 4 files changed, 16 insertions(+), 13 deletions(-)
drivers/cpuidle/cpuidle.c +0 -1
@@ -265,7 +265,6 @@ static void poll_idle_init(struct cpuidle_driver *drv) state->power_usage =-1 ; state->flags =0 ; state->enter = poll_idle; - state->disable = 0; }#else static void poll_idle_init (struct cpuidle_driver *drv) {}
drivers/cpuidle/governors/menu.c +3 -2
@@ -281,7 +281,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)* unless the timer is happening really really soon. */ if (data->expected_us >5 && - drv->states[CPUIDLE_DRIVER_STATE_START ].disable ==0 ) + dev->states_usage[CPUIDLE_DRIVER_STATE_START ].disable ==0 ) data->last_state_idx =CPUIDLE_DRIVER_STATE_START ;/*
@@ -290,8 +290,9 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)*/ 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->disable) +if (su->disable)continue ;if (s->target_residency > data->predicted_us)continue ;
drivers/cpuidle/sysfs.c +12 -9
@@ -217,7 +217,8 @@ struct cpuidle_state_attr {struct attribute attr;ssize_t (*show)(struct cpuidle_state *, \struct cpuidle_state_usage *,char *); -ssize_t (*store)(struct cpuidle_state *,const char *,size_t ); +ssize_t (*store)(struct cpuidle_state *, \ +struct cpuidle_state_usage *,const char *,size_t ); };#define define_one_state_ro (_name, show) \
@@ -233,21 +234,22 @@ static ssize_t show_state_##_name(struct cpuidle_state *state, \return sprintf (buf,"%u\n" , state->_name);\ } -#define define_store_state_function (_name) \ +#define define_store_state_ull_function (_name) \ static ssize_t store_state_##_name(struct cpuidle_state *state, \ + struct cpuidle_state_usage *state_usage, \ const char *buf, size_t size) \ { \ - long value; \ + unsigned long long value; \ int err; \ if (!capable(CAP_SYS_ADMIN)) \ return -EPERM; \ - err = kstrtol(buf, 0, &value); \ + err = kstrtoull(buf, 0, &value); \ if (err) \ return err; \ if (value) \ - state->disable = 1; \ + state_usage->_name = 1; \ else \ - state->disable = 0; \ + state_usage->_name = 0; \ return size; \ }
@@ -273,8 +275,8 @@ define_show_state_ull_function(usage)define_show_state_ull_function (time) define_show_state_str_function(name) define_show_state_str_function(desc) -define_show_state_function(disable) -define_store_state_function(disable) +define_show_state_ull_function(disable) +define_store_state_ull_function(disable) define_one_state_ro(name, show_state_name);define_one_state_ro (desc, show_state_desc);
@@ -318,10 +320,11 @@ static ssize_t cpuidle_state_store(struct kobject *kobj, {int ret = -EIO ;struct cpuidle_state *state =kobj_to_state (kobj); +struct cpuidle_state_usage *state_usage =kobj_to_state_usage (kobj);struct cpuidle_state_attr *cattr =attr_to_stateattr (attr);if (cattr->store) - ret = cattr->store (state, buf, size); + ret = cattr->store (state, state_usage, buf, size);return ret; }
include/linux/cpuidle.h +1 -1
@@ -34,6 +34,7 @@ struct cpuidle_driver;struct cpuidle_state_usage {void *driver_data; +unsigned long long disable;unsigned long long usage;unsigned long long time;/* in US */ };
@@ -46,7 +47,6 @@ struct cpuidle_state {unsigned int exit_latency;/* in US */ int power_usage;/* in mW */ unsigned int target_residency;/* in US */ -unsigned int disable; int (*enter) (struct cpuidle_device *dev, struct cpuidle_driver *drv,
Signature · 1 line
1.7.1
Hi,
Quoted 7 lines · ShuoX Liu
Andrew J.Schorr raises a question. When he changes the disable setting on a single CPU, it affects all the other CPUs. Basically, currently, the disable field is per-driver instead of per-cpu. All the C states of the same driver are shared by all CPU in the same machine.
Below patch changes field disable to per-cpu, so we could set this separately for each cpu.
Thanks. I have not tested this, but it looks very useful to have the flexibility to disable certain C states on a subset of the cores. The current implementation affecting all cores is much less powerful.
Thanks,
Andrew
Quoted 10 lines · ShuoX Liu
From: ShuoX Liu <[email protected]>
Andrew J.Schorr raises a question. When he changes the disable setting on a single CPU, it affects all the other CPUs. Basically, currently, the disable field is per-driver instead of per-cpu. All the C states of the same driver are shared by all CPU in the same machine.
Below patch changes field disable to per-cpu, so we could set this separately for each cpu.
This would help us have asymmetric C-states on cpus.
Quoted 3 lines
Reported-by Andrew J.Schorr <[email protected]>Reviewed-by Yanmin Zhang <[email protected]>Signed-off-by ShuoX Liu <[email protected]>
Quoted 129 lines
--- drivers/cpuidle/cpuidle.c | 1 - drivers/cpuidle/governors/menu.c | 5 +++-- drivers/cpuidle/sysfs.c | 21 ++++++++++++--------- include/linux/cpuidle.h | 2 +- 4 files changed, 16 insertions(+), 13 deletions(-)drivers/cpuidle/cpuidle.c +0
-1@@ -265,7 +265,6 @@ static void poll_idle_init(struct cpuidle_driver *drv) state->power_usage =-1 ; state->flags =0 ; state->enter = poll_idle; - state->disable = 0; }#else static void poll_idle_init (struct cpuidle_driver *drv) {}drivers/cpuidle/governors/menu.c +3
-2@@ -281,7 +281,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)* unless the timer is happening really really soon. */ if (data->expected_us >5 && - drv->states[CPUIDLE_DRIVER_STATE_START ].disable ==0 ) + dev->states_usage[CPUIDLE_DRIVER_STATE_START ].disable ==0 ) data->last_state_idx =CPUIDLE_DRIVER_STATE_START ;/* @@ -290,8 +290,9 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)*/ 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->disable) +if (su->disable)continue ;if (s->target_residency > data->predicted_us)continue ;drivers/cpuidle/sysfs.c +12
-9@@ -217,7 +217,8 @@ struct cpuidle_state_attr {struct attribute attr;ssize_t (*show)(struct cpuidle_state *, \struct cpuidle_state_usage *,char *); -ssize_t (*store)(struct cpuidle_state *,const char *,size_t ); +ssize_t (*store)(struct cpuidle_state *, \ +struct cpuidle_state_usage *,const char *,size_t ); };#define define_one_state_ro (_name, show) \@@ -233,21 +234,22 @@ static ssize_t show_state_##_name(struct cpuidle_state *state, \return sprintf (buf,"%u\n" , state->_name);\ } -#define define_store_state_function (_name) \ +#define define_store_state_ull_function (_name) \ static ssize_t store_state_##_name(struct cpuidle_state *state, \ + struct cpuidle_state_usage *state_usage, \ const char *buf, size_t size) \ { \ - long value; \ + unsigned long long value; \ int err; \ if (!capable(CAP_SYS_ADMIN)) \ return -EPERM; \ - err = kstrtol(buf, 0, &value); \ + err = kstrtoull(buf, 0, &value); \ if (err) \ return err; \ if (value) \ - state->disable = 1; \ + state_usage->_name = 1; \ else \ - state->disable = 0; \ + state_usage->_name = 0; \ return size; \ }@@ -273,8 +275,8 @@ define_show_state_ull_function(usage)define_show_state_ull_function (time) define_show_state_str_function(name) define_show_state_str_function(desc) -define_show_state_function(disable) -define_store_state_function(disable) +define_show_state_ull_function(disable) +define_store_state_ull_function(disable) define_one_state_ro(name, show_state_name);define_one_state_ro (desc, show_state_desc);@@ -318,10 +320,11 @@ static ssize_t cpuidle_state_store(struct kobject *kobj, {int ret = -EIO ;struct cpuidle_state *state =kobj_to_state (kobj); +struct cpuidle_state_usage *state_usage =kobj_to_state_usage (kobj);struct cpuidle_state_attr *cattr =attr_to_stateattr (attr);if (cattr->store) - ret = cattr->store (state, buf, size); + ret = cattr->store (state, state_usage, buf, size);return ret; }include/linux/cpuidle.h +1
-1@@ -34,6 +34,7 @@ struct cpuidle_driver;struct cpuidle_state_usage {void *driver_data; +unsigned long long disable;unsigned long long usage;unsigned long long time;/* in US */ };@@ -46,7 +47,6 @@ struct cpuidle_state {unsigned int exit_latency;/* in US */ int power_usage;/* in mW */ unsigned int target_residency;/* in US */ -unsigned int disable; int (*enter) (struct cpuidle_device *dev, struct cpuidle_driver *drv,
Quoted 14 lines · Deepthi Dharwar
Quoted 10 lines · ShuoX Liu
From: ShuoX Liu <[email protected]>
Andrew J.Schorr raises a question. When he changes the disable setting on a single CPU, it affects all the other CPUs. Basically, currently, the disable field is per-driver instead of per-cpu. All the C states of the same driver are shared by all CPU in the same machine.
Below patch changes field disable to per-cpu, so we could set this separately for each cpu.
Quoted 8 lines
This would help us have asymmetric C-states on cpus.
Quoted 3 lines
Reported-by Andrew J.Schorr <[email protected]>Reviewed-by Yanmin Zhang <[email protected]>Signed-off-by ShuoX Liu <[email protected]>Acked-by Deepthi Dharwar <[email protected]>
Len,
Would you like to accept the patch into your testing tree? Both Deepthi Dharwar and Andrew J. Schorr acked the patch is useful.
Quoted 137 lines
Quoted 123 lines
--- drivers/cpuidle/cpuidle.c | 1 - drivers/cpuidle/governors/menu.c | 5 +++-- drivers/cpuidle/sysfs.c | 21 ++++++++++++--------- include/linux/cpuidle.h | 2 +- 4 files changed, 16 insertions(+), 13 deletions(-)drivers/cpuidle/cpuidle.c +0
-1@@ -265,7 +265,6 @@ static void poll_idle_init(struct cpuidle_driver *drv) state->power_usage =-1 ; state->flags =0 ; state->enter = poll_idle; - state->disable = 0; }#else static void poll_idle_init (struct cpuidle_driver *drv) {}drivers/cpuidle/governors/menu.c +3
-2@@ -281,7 +281,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)* unless the timer is happening really really soon. */ if (data->expected_us >5 && - drv->states[CPUIDLE_DRIVER_STATE_START ].disable ==0 ) + dev->states_usage[CPUIDLE_DRIVER_STATE_START ].disable ==0 ) data->last_state_idx =CPUIDLE_DRIVER_STATE_START ;/* @@ -290,8 +290,9 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)*/ 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->disable) +if (su->disable)continue ;if (s->target_residency > data->predicted_us)continue ;drivers/cpuidle/sysfs.c +12
-9@@ -217,7 +217,8 @@ struct cpuidle_state_attr {struct attribute attr;ssize_t (*show)(struct cpuidle_state *, \struct cpuidle_state_usage *,char *); -ssize_t (*store)(struct cpuidle_state *,const char *,size_t ); +ssize_t (*store)(struct cpuidle_state *, \ +struct cpuidle_state_usage *,const char *,size_t ); };#define define_one_state_ro (_name, show) \@@ -233,21 +234,22 @@ static ssize_t show_state_##_name(struct cpuidle_state *state, \return sprintf (buf,"%u\n" , state->_name);\ } -#define define_store_state_function (_name) \ +#define define_store_state_ull_function (_name) \ static ssize_t store_state_##_name(struct cpuidle_state *state, \ + struct cpuidle_state_usage *state_usage, \ const char *buf, size_t size) \ { \ - long value; \ + unsigned long long value; \ int err; \ if (!capable(CAP_SYS_ADMIN)) \ return -EPERM; \ - err = kstrtol(buf, 0, &value); \ + err = kstrtoull(buf, 0, &value); \ if (err) \ return err; \ if (value) \ - state->disable = 1; \ + state_usage->_name = 1; \ else \ - state->disable = 0; \ + state_usage->_name = 0; \ return size; \ }@@ -273,8 +275,8 @@ define_show_state_ull_function(usage)define_show_state_ull_function (time) define_show_state_str_function(name) define_show_state_str_function(desc) -define_show_state_function(disable) -define_store_state_function(disable) +define_show_state_ull_function(disable) +define_store_state_ull_function(disable) define_one_state_ro(name, show_state_name);define_one_state_ro (desc, show_state_desc);@@ -318,10 +320,11 @@ static ssize_t cpuidle_state_store(struct kobject *kobj, {int ret = -EIO ;struct cpuidle_state *state =kobj_to_state (kobj); +struct cpuidle_state_usage *state_usage =kobj_to_state_usage (kobj);struct cpuidle_state_attr *cattr =attr_to_stateattr (attr);if (cattr->store) - ret = cattr->store (state, buf, size); + ret = cattr->store (state, state_usage, buf, size);return ret; }include/linux/cpuidle.h +1
-0@@ -34,6 +34,7 @@ struct cpuidle_driver;struct cpuidle_state_usage {void *driver_data; +unsigned long long disable;unsigned long long usage;unsigned long long time;/* in US */ };include/linux/cpuidle.h struct cpuidle_state { · line 52
Quoted 7 lines
include/linux/cpuidle.h +0
-1@@ -46,7 +47,6 @@ struct cpuidle_state {unsigned int exit_latency;/* in US */ int power_usage;/* in mW */ unsigned int target_residency;/* in US */ -unsigned int disable; int (*enter) (struct cpuidle_device *dev, struct cpuidle_driver *drv,--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Quoted 15 lines · On Thu, 14 Jun 2012 10:52:48 +0800 ShuoX Liu <[email protected]> wrote:
From: ShuoX Liu <[email protected]>
Andrew J.Schorr raises a question. When he changes the disable setting on a single CPU, it affects all the other CPUs. Basically, currently, the disable field is per-driver instead of per-cpu. All the C states of the same driver are shared by all CPU in the same machine.
Below patch changes field disable to per-cpu, so we could set this separately for each cpu.
...
include/linux/cpuidle.h struct cpuidle_driver; · line 37
Quoted 4 lines
include/linux/cpuidle.h +1
-0@@ -34,6 +34,7 @@ struct cpuidle_driver;struct cpuidle_state_usage {void *driver_data; +unsigned long long disable;
hrmpf. We're using 64 bits for this where one bit would do, afaict because the magic macros in drivers/cpuidle/sysfs.c are using %llu.
Andrew,
You are right. It's just to reuse current functions, which show 64bit usage/time. Should we add 2 new macros to define new functions for int?
Yanmin
-----Original Message-----
From: Andrew Morton [mailto:[email protected]]
Sent: Saturday, June 23, 2012 7:10 AM
To: Liu, ShuoX
Cc: [email protected]; Brown, Len; Zhang, Yanmin; Andrew J. Schorr; Deepthi Dharwar
Subject: Re: [PATCH] cpuidle: move field disable from per-driver to per-cpu
Quoted 15 lines · On Thu, 14 Jun 2012 10:52:48 +0800 ShuoX Liu <[email protected]> wrote:
From: ShuoX Liu <[email protected]>
Andrew J.Schorr raises a question. When he changes the disable setting on a single CPU, it affects all the other CPUs. Basically, currently, the disable field is per-driver instead of per-cpu. All the C states of the same driver are shared by all CPU in the same machine.
Below patch changes field disable to per-cpu, so we could set this separately for each cpu.
...
include/linux/cpuidle.h struct cpuidle_driver; struct cpuidle_state_usage { · line 36
Quoted 3 lines
include/linux/cpuidle.h +1
-0@@ -34,6 +34,7 @@ struct cpuidle_driver; struct cpuidle_state_usage {void *driver_data; +unsigned long long disable;
hrmpf. We're using 64 bits for this where one bit would do, afaict because the magic macros in drivers/cpuidle/sysfs.c are using %llu.