Currently, in intel_idle.c, there are 5 state_tables array, every
array size is sizeof(struct cpuidle_state) * CPUIDLE_STATE_MAX.
But after intel_idle_probe(), just only one array is useful.
Here we can just define one static state_table, and initialize it
in intel_idle_probe(), and set other data state_tables as __initdata.
It can save about 3K, which also benefits mobile devices.
Signed-off-by: liu chuansheng <[email protected]>
---
drivers/idle/intel_idle.c | 19 ++++++++++++-------
1 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 5d66750..0642bfe 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -99,7 +99,7 @@ static int intel_idle(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int index);
static int intel_idle_cpu_init(int cpu);
-static struct cpuidle_state *cpuidle_state_table;
+static struct cpuidle_state cpuidle_state_table[CPUIDLE_STATE_MAX];
/*
* Set this flag for states where the HW flushes the TLB for us
@@ -124,7 +124,7 @@ static struct cpuidle_state *cpuidle_state_table;
* which is also the index into the MWAIT hint array.
* Thus C0 is a dummy.
*/
-static struct cpuidle_state nehalem_cstates[CPUIDLE_STATE_MAX] = {
+static struct cpuidle_state nehalem_cstates[CPUIDLE_STATE_MAX] __initdata = {
{
.name = "C1-NHM",
.desc = "MWAIT 0x00",
@@ -157,7 +157,7 @@ static struct cpuidle_state nehalem_cstates[CPUIDLE_STATE_MAX] = {
.enter = NULL }
};
-static struct cpuidle_state snb_cstates[CPUIDLE_STATE_MAX] = {
+static struct cpuidle_state snb_cstates[CPUIDLE_STATE_MAX] __initdata = {
{
.name = "C1-SNB",
.desc = "MWAIT 0x00",
@@ -197,7 +197,7 @@ static struct cpuidle_state snb_cstates[CPUIDLE_STATE_MAX] = {
.enter = NULL }
};
-static struct cpuidle_state ivb_cstates[CPUIDLE_STATE_MAX] = {
+static struct cpuidle_state ivb_cstates[CPUIDLE_STATE_MAX] __initdata = {
{
.name = "C1-IVB",
.desc = "MWAIT 0x00",
@@ -237,7 +237,7 @@ static struct cpuidle_state ivb_cstates[CPUIDLE_STATE_MAX] = {
.enter = NULL }
};
-static struct cpuidle_state hsw_cstates[CPUIDLE_STATE_MAX] = {
+static struct cpuidle_state hsw_cstates[CPUIDLE_STATE_MAX] __initdata = {
{
.name = "C1-HSW",
.desc = "MWAIT 0x00",
@@ -277,7 +277,7 @@ static struct cpuidle_state hsw_cstates[CPUIDLE_STATE_MAX] = {
.enter = NULL }
};
-static struct cpuidle_state atom_cstates[CPUIDLE_STATE_MAX] = {
+static struct cpuidle_state atom_cstates[CPUIDLE_STATE_MAX] __initdata = {
{
.name = "C1E-ATM",
.desc = "MWAIT 0x00",
@@ -504,7 +504,12 @@ static int intel_idle_probe(void)
pr_debug(PREFIX "MWAIT substates: 0x%x\n", mwait_substates);
icpu = (const struct idle_cpu *)id->driver_data;
- cpuidle_state_table = icpu->state_table;
+ /* Copy the icpu->state_table into cpuidle_state_table,
+ * The pointing array by icpu->state_table is with __initdata,
+ * which will be freed after kernel init ending.
+ */
+ memcpy(cpuidle_state_table, icpu->state_table,
+ sizeof(cpuidle_state_table));
if (boot_cpu_has(X86_FEATURE_ARAT)) /* Always Reliable APIC Timer */
lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
--
1.7.0.4
On 03/07/2013 05:42 PM, Chuansheng Liu wrote:
>
> Currently, in intel_idle.c, there are 5 state_tables array, every
> array size is sizeof(struct cpuidle_state) * CPUIDLE_STATE_MAX.
>
> But after intel_idle_probe(), just only one array is useful.
>
> Here we can just define one static state_table, and initialize it
> in intel_idle_probe(), and set other data state_tables as __initdata.
>
> It can save about 3K, which also benefits mobile devices.
>
> Signed-off-by: liu chuansheng <[email protected]>
> ---
> drivers/idle/intel_idle.c | 19 ++++++++++++-------
> 1 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 5d66750..0642bfe 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -99,7 +99,7 @@ static int intel_idle(struct cpuidle_device *dev,
> struct cpuidle_driver *drv, int index);
> static int intel_idle_cpu_init(int cpu);
>
> -static struct cpuidle_state *cpuidle_state_table;
> +static struct cpuidle_state cpuidle_state_table[CPUIDLE_STATE_MAX];
>
> /*
> * Set this flag for states where the HW flushes the TLB for us
> @@ -124,7 +124,7 @@ static struct cpuidle_state *cpuidle_state_table;
> * which is also the index into the MWAIT hint array.
> * Thus C0 is a dummy.
> */
> -static struct cpuidle_state nehalem_cstates[CPUIDLE_STATE_MAX] = {
> +static struct cpuidle_state nehalem_cstates[CPUIDLE_STATE_MAX] __initdata = {
> {
> .name = "C1-NHM",
> .desc = "MWAIT 0x00",
> @@ -157,7 +157,7 @@ static struct cpuidle_state nehalem_cstates[CPUIDLE_STATE_MAX] = {
> .enter = NULL }
> };
>
> -static struct cpuidle_state snb_cstates[CPUIDLE_STATE_MAX] = {
> +static struct cpuidle_state snb_cstates[CPUIDLE_STATE_MAX] __initdata = {
> {
> .name = "C1-SNB",
> .desc = "MWAIT 0x00",
> @@ -197,7 +197,7 @@ static struct cpuidle_state snb_cstates[CPUIDLE_STATE_MAX] = {
> .enter = NULL }
> };
>
> -static struct cpuidle_state ivb_cstates[CPUIDLE_STATE_MAX] = {
> +static struct cpuidle_state ivb_cstates[CPUIDLE_STATE_MAX] __initdata = {
> {
> .name = "C1-IVB",
> .desc = "MWAIT 0x00",
> @@ -237,7 +237,7 @@ static struct cpuidle_state ivb_cstates[CPUIDLE_STATE_MAX] = {
> .enter = NULL }
> };
>
> -static struct cpuidle_state hsw_cstates[CPUIDLE_STATE_MAX] = {
> +static struct cpuidle_state hsw_cstates[CPUIDLE_STATE_MAX] __initdata = {
> {
> .name = "C1-HSW",
> .desc = "MWAIT 0x00",
> @@ -277,7 +277,7 @@ static struct cpuidle_state hsw_cstates[CPUIDLE_STATE_MAX] = {
> .enter = NULL }
> };
>
> -static struct cpuidle_state atom_cstates[CPUIDLE_STATE_MAX] = {
> +static struct cpuidle_state atom_cstates[CPUIDLE_STATE_MAX] __initdata = {
> {
> .name = "C1E-ATM",
> .desc = "MWAIT 0x00",
> @@ -504,7 +504,12 @@ static int intel_idle_probe(void)
> pr_debug(PREFIX "MWAIT substates: 0x%x\n", mwait_substates);
>
> icpu = (const struct idle_cpu *)id->driver_data;
> - cpuidle_state_table = icpu->state_table;
> + /* Copy the icpu->state_table into cpuidle_state_table,
> + * The pointing array by icpu->state_table is with __initdata,
> + * which will be freed after kernel init ending.
> + */
> + memcpy(cpuidle_state_table, icpu->state_table,
> + sizeof(cpuidle_state_table));
The idea is good but it could be pushed a bit further.
Instead of changing cpuidle_state_table to an array, change it to an
initdata and the functions intel_idle_cpuidle_driver_init,
intel_idle_probe to init functions.
That would also be nice to get rid of a global variable to store the
current cpu data and let the intel_idle_probe to return the pointer to
the right table.
> if (boot_cpu_has(X86_FEATURE_ARAT)) /* Always Reliable APIC Timer */
> lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
> -----Original Message-----
> From: Daniel Lezcano [mailto:[email protected]]
> Sent: Thursday, March 07, 2013 5:49 PM
> To: Liu, Chuansheng
> Cc: [email protected]; Brown, Len; [email protected];
> [email protected]
> Subject: Re: [PATCH] intel_idle: set the state_tables array into __initdata to
> save mem
>
> On 03/07/2013 05:42 PM, Chuansheng Liu wrote:
> >
> > Currently, in intel_idle.c, there are 5 state_tables array, every
> > array size is sizeof(struct cpuidle_state) * CPUIDLE_STATE_MAX.
> >
> > But after intel_idle_probe(), just only one array is useful.
> >
> > Here we can just define one static state_table, and initialize it
> > in intel_idle_probe(), and set other data state_tables as __initdata.
> >
> > It can save about 3K, which also benefits mobile devices.
> >
> > Signed-off-by: liu chuansheng <[email protected]>
> > ---
> > drivers/idle/intel_idle.c | 19 ++++++++++++-------
> > 1 files changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> > index 5d66750..0642bfe 100644
> > --- a/drivers/idle/intel_idle.c
> > +++ b/drivers/idle/intel_idle.c
> > @@ -99,7 +99,7 @@ static int intel_idle(struct cpuidle_device *dev,
> > struct cpuidle_driver *drv, int index);
> > static int intel_idle_cpu_init(int cpu);
> >
> > -static struct cpuidle_state *cpuidle_state_table;
> > +static struct cpuidle_state cpuidle_state_table[CPUIDLE_STATE_MAX];
> >
> > /*
> > * Set this flag for states where the HW flushes the TLB for us
> > @@ -124,7 +124,7 @@ static struct cpuidle_state *cpuidle_state_table;
> > * which is also the index into the MWAIT hint array.
> > * Thus C0 is a dummy.
> > */
> > -static struct cpuidle_state nehalem_cstates[CPUIDLE_STATE_MAX] = {
> > +static struct cpuidle_state nehalem_cstates[CPUIDLE_STATE_MAX]
> __initdata = {
> > {
> > .name = "C1-NHM",
> > .desc = "MWAIT 0x00",
> > @@ -157,7 +157,7 @@ static struct cpuidle_state
> nehalem_cstates[CPUIDLE_STATE_MAX] = {
> > .enter = NULL }
> > };
> >
> > -static struct cpuidle_state snb_cstates[CPUIDLE_STATE_MAX] = {
> > +static struct cpuidle_state snb_cstates[CPUIDLE_STATE_MAX] __initdata = {
> > {
> > .name = "C1-SNB",
> > .desc = "MWAIT 0x00",
> > @@ -197,7 +197,7 @@ static struct cpuidle_state
> snb_cstates[CPUIDLE_STATE_MAX] = {
> > .enter = NULL }
> > };
> >
> > -static struct cpuidle_state ivb_cstates[CPUIDLE_STATE_MAX] = {
> > +static struct cpuidle_state ivb_cstates[CPUIDLE_STATE_MAX] __initdata = {
> > {
> > .name = "C1-IVB",
> > .desc = "MWAIT 0x00",
> > @@ -237,7 +237,7 @@ static struct cpuidle_state
> ivb_cstates[CPUIDLE_STATE_MAX] = {
> > .enter = NULL }
> > };
> >
> > -static struct cpuidle_state hsw_cstates[CPUIDLE_STATE_MAX] = {
> > +static struct cpuidle_state hsw_cstates[CPUIDLE_STATE_MAX] __initdata =
> {
> > {
> > .name = "C1-HSW",
> > .desc = "MWAIT 0x00",
> > @@ -277,7 +277,7 @@ static struct cpuidle_state
> hsw_cstates[CPUIDLE_STATE_MAX] = {
> > .enter = NULL }
> > };
> >
> > -static struct cpuidle_state atom_cstates[CPUIDLE_STATE_MAX] = {
> > +static struct cpuidle_state atom_cstates[CPUIDLE_STATE_MAX] __initdata
> = {
> > {
> > .name = "C1E-ATM",
> > .desc = "MWAIT 0x00",
> > @@ -504,7 +504,12 @@ static int intel_idle_probe(void)
> > pr_debug(PREFIX "MWAIT substates: 0x%x\n", mwait_substates);
> >
> > icpu = (const struct idle_cpu *)id->driver_data;
> > - cpuidle_state_table = icpu->state_table;
> > + /* Copy the icpu->state_table into cpuidle_state_table,
> > + * The pointing array by icpu->state_table is with __initdata,
> > + * which will be freed after kernel init ending.
> > + */
> > + memcpy(cpuidle_state_table, icpu->state_table,
> > + sizeof(cpuidle_state_table));
>
> The idea is good but it could be pushed a bit further.
>
> Instead of changing cpuidle_state_table to an array, change it to an
> initdata and the functions intel_idle_cpuidle_driver_init,
> intel_idle_probe to init functions.
You are right. The global variable intel_idle_driver will has the copy of cpuidle_state_table[],
will update the new patch. Thanks.
>
> That would also be nice to get rid of a global variable to store the
> current cpu data and let the intel_idle_probe to return the pointer to
> the right table.
Sounds reasonable, will have a try also. Thanks again.
>
>
> > if (boot_cpu_has(X86_FEATURE_ARAT)) /* Always Reliable APIC Timer */
> > lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
> >
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
As Daniel suggested, I did some cleanup before setting the state_tables array
into __initdata.
Thanks your help to review them.
[PATCH 1/3] intel_idle: changing the continue to break in intel_idle_cpu_init()
[PATCH 2/3] intel_idle: Removing the redundant calculating for dev->state_count
[PATCH 3/3] intel_idle: set the state_tables array as __initdata to save mem
According to commit e022e7eb9, the .enter == NULL is the last one in
state_tables[].
So just like intel_idle_cpuidle_driver_init(), in case of .enter == NULL,
breaking the for(;;) loop directly.
Signed-off-by: liu chuansheng <[email protected]>
---
drivers/idle/intel_idle.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 5d66750..17c9cf9 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -610,7 +610,7 @@ static int intel_idle_cpu_init(int cpu)
int num_substates, mwait_hint, mwait_cstate, mwait_substate;
if (cpuidle_state_table[cstate].enter == NULL)
- continue;
+ break;
if (cstate + 1 > max_cstate) {
printk(PREFIX "max_cstate %d reached\n", max_cstate);
--
1.7.0.4
In function intel_idle_cpu_init() and intel_idle_cpuidle_driver_init(),
they are having the same for(;;) loop.
Here in intel_idle_cpu_init(), the dev->state_count can be assigned by
drv->state_count directly.
Signed-off-by: liu chuansheng <[email protected]>
---
drivers/idle/intel_idle.c | 30 ++----------------------------
1 files changed, 2 insertions(+), 28 deletions(-)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 17c9cf9..503b401 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -599,38 +599,12 @@ static int intel_idle_cpuidle_driver_init(void)
*/
static int intel_idle_cpu_init(int cpu)
{
- int cstate;
struct cpuidle_device *dev;
+ struct cpuidle_driver *drv = &intel_idle_driver;
dev = per_cpu_ptr(intel_idle_cpuidle_devices, cpu);
- dev->state_count = 1;
-
- for (cstate = 0; cstate < CPUIDLE_STATE_MAX; ++cstate) {
- int num_substates, mwait_hint, mwait_cstate, mwait_substate;
-
- if (cpuidle_state_table[cstate].enter == NULL)
- break;
-
- if (cstate + 1 > max_cstate) {
- printk(PREFIX "max_cstate %d reached\n", max_cstate);
- break;
- }
-
- mwait_hint = flg2MWAIT(cpuidle_state_table[cstate].flags);
- mwait_cstate = MWAIT_HINT2CSTATE(mwait_hint);
- mwait_substate = MWAIT_HINT2SUBSTATE(mwait_hint);
-
- /* does the state exist in CPUID.MWAIT? */
- num_substates = (mwait_substates >> ((mwait_cstate + 1) * 4))
- & MWAIT_SUBSTATE_MASK;
-
- /* if sub-state in table is not enumerated by CPUID */
- if ((mwait_substate + 1) > num_substates)
- continue;
-
- dev->state_count += 1;
- }
+ dev->state_count = drv->state_count;
dev->cpu = cpu;
--
1.7.0.4
Currently, in intel_idle.c, there are 5 state_tables array, every
array size is sizeof(struct cpuidle_state) * CPUIDLE_STATE_MAX.
As in intel_idle_cpuidle_driver_init(), we have copied the data into
intel_idle_driver->state[], so do not need to keep state_tables[]
there any more after system init.
It will save about 3~4k memory, also benefits mobile devices.
Here changing them as __initdata, also removing global var
cpuidle_state_table pointer.
Signed-off-by: liu chuansheng <[email protected]>
---
drivers/idle/intel_idle.c | 18 ++++++++----------
1 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 503b401..cbdc952 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -99,8 +99,6 @@ static int intel_idle(struct cpuidle_device *dev,
struct cpuidle_driver *drv, int index);
static int intel_idle_cpu_init(int cpu);
-static struct cpuidle_state *cpuidle_state_table;
-
/*
* Set this flag for states where the HW flushes the TLB for us
* and so we don't need cross-calls to keep it consistent.
@@ -124,7 +122,7 @@ static struct cpuidle_state *cpuidle_state_table;
* which is also the index into the MWAIT hint array.
* Thus C0 is a dummy.
*/
-static struct cpuidle_state nehalem_cstates[CPUIDLE_STATE_MAX] = {
+static struct cpuidle_state nehalem_cstates[CPUIDLE_STATE_MAX] __initdata = {
{
.name = "C1-NHM",
.desc = "MWAIT 0x00",
@@ -157,7 +155,7 @@ static struct cpuidle_state nehalem_cstates[CPUIDLE_STATE_MAX] = {
.enter = NULL }
};
-static struct cpuidle_state snb_cstates[CPUIDLE_STATE_MAX] = {
+static struct cpuidle_state snb_cstates[CPUIDLE_STATE_MAX] __initdata = {
{
.name = "C1-SNB",
.desc = "MWAIT 0x00",
@@ -197,7 +195,7 @@ static struct cpuidle_state snb_cstates[CPUIDLE_STATE_MAX] = {
.enter = NULL }
};
-static struct cpuidle_state ivb_cstates[CPUIDLE_STATE_MAX] = {
+static struct cpuidle_state ivb_cstates[CPUIDLE_STATE_MAX] __initdata = {
{
.name = "C1-IVB",
.desc = "MWAIT 0x00",
@@ -237,7 +235,7 @@ static struct cpuidle_state ivb_cstates[CPUIDLE_STATE_MAX] = {
.enter = NULL }
};
-static struct cpuidle_state hsw_cstates[CPUIDLE_STATE_MAX] = {
+static struct cpuidle_state hsw_cstates[CPUIDLE_STATE_MAX] __initdata = {
{
.name = "C1-HSW",
.desc = "MWAIT 0x00",
@@ -277,7 +275,7 @@ static struct cpuidle_state hsw_cstates[CPUIDLE_STATE_MAX] = {
.enter = NULL }
};
-static struct cpuidle_state atom_cstates[CPUIDLE_STATE_MAX] = {
+static struct cpuidle_state atom_cstates[CPUIDLE_STATE_MAX] __initdata = {
{
.name = "C1E-ATM",
.desc = "MWAIT 0x00",
@@ -472,7 +470,7 @@ MODULE_DEVICE_TABLE(x86cpu, intel_idle_ids);
/*
* intel_idle_probe()
*/
-static int intel_idle_probe(void)
+static int __init intel_idle_probe(void)
{
unsigned int eax, ebx, ecx;
const struct x86_cpu_id *id;
@@ -504,7 +502,6 @@ static int intel_idle_probe(void)
pr_debug(PREFIX "MWAIT substates: 0x%x\n", mwait_substates);
icpu = (const struct idle_cpu *)id->driver_data;
- cpuidle_state_table = icpu->state_table;
if (boot_cpu_has(X86_FEATURE_ARAT)) /* Always Reliable APIC Timer */
lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
@@ -540,10 +537,11 @@ static void intel_idle_cpuidle_devices_uninit(void)
* intel_idle_cpuidle_driver_init()
* allocate, initialize cpuidle_states
*/
-static int intel_idle_cpuidle_driver_init(void)
+static int __init intel_idle_cpuidle_driver_init(void)
{
int cstate;
struct cpuidle_driver *drv = &intel_idle_driver;
+ struct cpuidle_state *cpuidle_state_table = icpu->state_table;
drv->state_count = 1;
--
1.7.0.4
On 03/08/2013 04:04 PM, Chuansheng Liu wrote:
>
> In function intel_idle_cpu_init() and intel_idle_cpuidle_driver_init(),
> they are having the same for(;;) loop.
>
> Here in intel_idle_cpu_init(), the dev->state_count can be assigned by
> drv->state_count directly.
>
> Signed-off-by: liu chuansheng <[email protected]>
> ---
> drivers/idle/intel_idle.c | 30 ++----------------------------
> 1 files changed, 2 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 17c9cf9..503b401 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -599,38 +599,12 @@ static int intel_idle_cpuidle_driver_init(void)
> */
> static int intel_idle_cpu_init(int cpu)
> {
> - int cstate;
> struct cpuidle_device *dev;
> + struct cpuidle_driver *drv = &intel_idle_driver;
>
> dev = per_cpu_ptr(intel_idle_cpuidle_devices, cpu);
>
> - dev->state_count = 1;
> -
> - for (cstate = 0; cstate < CPUIDLE_STATE_MAX; ++cstate) {
> - int num_substates, mwait_hint, mwait_cstate, mwait_substate;
> -
> - if (cpuidle_state_table[cstate].enter == NULL)
> - break;
> -
> - if (cstate + 1 > max_cstate) {
> - printk(PREFIX "max_cstate %d reached\n", max_cstate);
> - break;
> - }
> -
> - mwait_hint = flg2MWAIT(cpuidle_state_table[cstate].flags);
> - mwait_cstate = MWAIT_HINT2CSTATE(mwait_hint);
> - mwait_substate = MWAIT_HINT2SUBSTATE(mwait_hint);
> -
> - /* does the state exist in CPUID.MWAIT? */
> - num_substates = (mwait_substates >> ((mwait_cstate + 1) * 4))
> - & MWAIT_SUBSTATE_MASK;
> -
> - /* if sub-state in table is not enumerated by CPUID */
> - if ((mwait_substate + 1) > num_substates)
> - continue;
> -
> - dev->state_count += 1;
> - }
> + dev->state_count = drv->state_count;
The cpuidle_register_device function already does this initialization.
Probably you can get rid of this initialization and certainly factor out
a bit the code in this case.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 03/08/2013 04:06 PM, Chuansheng Liu wrote:
>
> Currently, in intel_idle.c, there are 5 state_tables array, every
> array size is sizeof(struct cpuidle_state) * CPUIDLE_STATE_MAX.
>
> As in intel_idle_cpuidle_driver_init(), we have copied the data into
> intel_idle_driver->state[], so do not need to keep state_tables[]
> there any more after system init.
>
> It will save about 3~4k memory, also benefits mobile devices.
> Here changing them as __initdata, also removing global var
> cpuidle_state_table pointer.
>
> Signed-off-by: liu chuansheng <[email protected]>
> ---
Acked-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On 03/08/2013 04:03 PM, Chuansheng Liu wrote:
>
> According to commit e022e7eb9, the .enter == NULL is the last one in
> state_tables[].
>
> So just like intel_idle_cpuidle_driver_init(), in case of .enter == NULL,
> breaking the for(;;) loop directly.
>
> Signed-off-by: liu chuansheng <[email protected]>
> ---
Sounds good.
Acked-by: Daniel Lezcano <[email protected]>
> drivers/idle/intel_idle.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index 5d66750..17c9cf9 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -610,7 +610,7 @@ static int intel_idle_cpu_init(int cpu)
> int num_substates, mwait_hint, mwait_cstate, mwait_substate;
>
> if (cpuidle_state_table[cstate].enter == NULL)
> - continue;
> + break;
>
> if (cstate + 1 > max_cstate) {
> printk(PREFIX "max_cstate %d reached\n", max_cstate);
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
> -----Original Message-----
> From: Daniel Lezcano [mailto:[email protected]]
> Sent: Saturday, March 09, 2013 11:00 AM
> To: Liu, Chuansheng
> Cc: [email protected]; Brown, Len; [email protected];
> [email protected]
> Subject: Re: [PATCH 2/3] intel_idle: Removing the redundant calculating for
> dev->state_count
>
> On 03/08/2013 04:04 PM, Chuansheng Liu wrote:
> >
> > In function intel_idle_cpu_init() and intel_idle_cpuidle_driver_init(),
> > they are having the same for(;;) loop.
> >
> > Here in intel_idle_cpu_init(), the dev->state_count can be assigned by
> > drv->state_count directly.
> >
> > Signed-off-by: liu chuansheng <[email protected]>
> > ---
> > drivers/idle/intel_idle.c | 30 ++----------------------------
> > 1 files changed, 2 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> > index 17c9cf9..503b401 100644
> > --- a/drivers/idle/intel_idle.c
> > +++ b/drivers/idle/intel_idle.c
> > @@ -599,38 +599,12 @@ static int intel_idle_cpuidle_driver_init(void)
> > */
> > static int intel_idle_cpu_init(int cpu)
> > {
> > - int cstate;
> > struct cpuidle_device *dev;
> > + struct cpuidle_driver *drv = &intel_idle_driver;
> >
> > dev = per_cpu_ptr(intel_idle_cpuidle_devices, cpu);
> >
> > - dev->state_count = 1;
> > -
> > - for (cstate = 0; cstate < CPUIDLE_STATE_MAX; ++cstate) {
> > - int num_substates, mwait_hint, mwait_cstate, mwait_substate;
> > -
> > - if (cpuidle_state_table[cstate].enter == NULL)
> > - break;
> > -
> > - if (cstate + 1 > max_cstate) {
> > - printk(PREFIX "max_cstate %d reached\n", max_cstate);
> > - break;
> > - }
> > -
> > - mwait_hint = flg2MWAIT(cpuidle_state_table[cstate].flags);
> > - mwait_cstate = MWAIT_HINT2CSTATE(mwait_hint);
> > - mwait_substate = MWAIT_HINT2SUBSTATE(mwait_hint);
> > -
> > - /* does the state exist in CPUID.MWAIT? */
> > - num_substates = (mwait_substates >> ((mwait_cstate + 1) * 4))
> > - & MWAIT_SUBSTATE_MASK;
> > -
> > - /* if sub-state in table is not enumerated by CPUID */
> > - if ((mwait_substate + 1) > num_substates)
> > - continue;
> > -
> > - dev->state_count += 1;
> > - }
> > + dev->state_count = drv->state_count;
>
> The cpuidle_register_device function already does this initialization.
Thanks your remind, will send patch V2 to remove it.
>
> Probably you can get rid of this initialization and certainly factor out
> a bit the code in this case.
>
>
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
In function intel_idle_cpu_init() and intel_idle_cpuidle_driver_init(),
they are having the same for(;;) loop to count the ->state_count.
Although intel_idle_cpu_init() can be called at runtime CPU HOTPLUG case,
but max_cstate can not be changed at runtime.
So the dev->state_count should be == drv->state_count, in the function
cpuidle_register_device() has done the initialization.
Here we can clean up these pieces of code.
Signed-off-by: liu chuansheng <[email protected]>
---
drivers/idle/intel_idle.c | 29 -----------------------------
1 files changed, 0 insertions(+), 29 deletions(-)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 17c9cf9..bb0ae0b 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -599,39 +599,10 @@ static int intel_idle_cpuidle_driver_init(void)
*/
static int intel_idle_cpu_init(int cpu)
{
- int cstate;
struct cpuidle_device *dev;
dev = per_cpu_ptr(intel_idle_cpuidle_devices, cpu);
- dev->state_count = 1;
-
- for (cstate = 0; cstate < CPUIDLE_STATE_MAX; ++cstate) {
- int num_substates, mwait_hint, mwait_cstate, mwait_substate;
-
- if (cpuidle_state_table[cstate].enter == NULL)
- break;
-
- if (cstate + 1 > max_cstate) {
- printk(PREFIX "max_cstate %d reached\n", max_cstate);
- break;
- }
-
- mwait_hint = flg2MWAIT(cpuidle_state_table[cstate].flags);
- mwait_cstate = MWAIT_HINT2CSTATE(mwait_hint);
- mwait_substate = MWAIT_HINT2SUBSTATE(mwait_hint);
-
- /* does the state exist in CPUID.MWAIT? */
- num_substates = (mwait_substates >> ((mwait_cstate + 1) * 4))
- & MWAIT_SUBSTATE_MASK;
-
- /* if sub-state in table is not enumerated by CPUID */
- if ((mwait_substate + 1) > num_substates)
- continue;
-
- dev->state_count += 1;
- }
-
dev->cpu = cpu;
if (cpuidle_register_device(dev)) {
--
1.7.0.4
On 03/11/2013 11:44 AM, Chuansheng Liu wrote:
>
> In function intel_idle_cpu_init() and intel_idle_cpuidle_driver_init(),
> they are having the same for(;;) loop to count the ->state_count.
>
> Although intel_idle_cpu_init() can be called at runtime CPU HOTPLUG case,
> but max_cstate can not be changed at runtime.
>
> So the dev->state_count should be == drv->state_count, in the function
> cpuidle_register_device() has done the initialization.
>
> Here we can clean up these pieces of code.
>
> Signed-off-by: liu chuansheng <[email protected]>
> ---
Acked-by: Daniel Lezcano <[email protected]>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog