[PATCH] intel_idle: set the state_tables array into __initdata to save mem

[PATCH] intel_idle: set the state_tables array into __initdata to save mem

[Chuansheng Liu]

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 <chuansheng.liu@intel.com>
---
 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

Re: [PATCH] intel_idle: set the state_tables array into __initdata to save mem

[Daniel Lezcano]

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 <chuansheng.liu@intel.com>
> ---
>  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

RE: [PATCH] intel_idle: set the state_tables array into __initdata to save mem

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4958 bytes --]



> -----Original Message-----
> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> Sent: Thursday, March 07, 2013 5:49 PM
> To: Liu, Chuansheng
> Cc: lenb@kernel.org; Brown, Len; linux-pm@vger.kernel.org;
> linux-kernel@vger.kernel.org
> 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 <chuansheng.liu@intel.com>
> > ---
> >  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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH 0/3] intel_idle: set the state_tables array into __initdata to save mem

[Chuansheng Liu]

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

[PATCH 1/3] intel_idle: changing the continue to break in intel_idle_cpu_init()

[Chuansheng Liu]

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 <chuansheng.liu@intel.com>
---
 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

Re: [PATCH 1/3] intel_idle: changing the continue to break in intel_idle_cpu_init()

[Daniel Lezcano]

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 <chuansheng.liu@intel.com>
> ---

Sounds good.

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

>  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

[PATCH 2/3] intel_idle: Removing the redundant calculating for dev->state_count

[Chuansheng Liu]

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 <chuansheng.liu@intel.com>
---
 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

Re: [PATCH 2/3] intel_idle: Removing the redundant calculating for dev->state_count

[Daniel Lezcano]

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 <chuansheng.liu@intel.com>
> ---
>  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

RE: [PATCH 2/3] intel_idle: Removing the redundant calculating for dev->state_count

[Liu, Chuansheng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2893 bytes --]



> -----Original Message-----
> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> Sent: Saturday, March 09, 2013 11:00 AM
> To: Liu, Chuansheng
> Cc: lenb@kernel.org; Brown, Len; linux-pm@vger.kernel.org;
> linux-kernel@vger.kernel.org
> 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 <chuansheng.liu@intel.com>
> > ---
> >  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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH 2/3 V2] intel_idle: Removing the redundant calculating for dev->state_count

[Chuansheng Liu]

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 <chuansheng.liu@intel.com>
---
 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

Re: [PATCH 2/3 V2] intel_idle: Removing the redundant calculating for dev->state_count

[Daniel Lezcano]

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 <chuansheng.liu@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>




-- 
 <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

[PATCH 3/3] intel_idle: set the state_tables array as __initdata to save mem

[Chuansheng Liu]

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 <chuansheng.liu@intel.com>
---
 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

Re: [PATCH 3/3] intel_idle: set the state_tables array as __initdata to save mem

[Daniel Lezcano]

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 <chuansheng.liu@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

-- 
 <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