diff mbox

intel_idle: set the state_tables array into __initdata to save mem

Message ID 1362674529.31506.17.camel@cliu38-desktop-build (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Chuansheng Liu March 7, 2013, 4:42 p.m. UTC
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(-)

Comments

Daniel Lezcano March 7, 2013, 9:49 a.m. UTC | #1
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;
>
Chuansheng Liu March 8, 2013, 12:44 a.m. UTC | #2
> -----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
Chuansheng Liu March 8, 2013, 3:02 p.m. UTC | #3
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


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;