[PATCH 1/4] acpi: intel_idle : break dependency between modules

[PATCH 1/4] acpi: intel_idle : break dependency between modules

[Daniel Lezcano]

When the system is booted with some cpus offline, the idle
driver is not initialized. When a cpu is set online, the
acpi code call the intel idle init function. Unfortunately
this code introduce a dependency between intel_idle and acpi.

This patch is intended to remove this dependency by using the
notifier of intel_idle. This patch has the benefit of
encapsulating the intel_idle driver and remove some exported
functions.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/processor_driver.c |    7 ------
 drivers/idle/intel_idle.c       |   41 +++++++++++++++++++++++++-------------
 include/linux/cpuidle.h         |    7 ------
 3 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 13103aeb..7048b97 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -429,18 +429,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
 		 * Initialize missing things
 		 */
 		if (pr->flags.need_hotplug_init) {
-			struct cpuidle_driver *idle_driver =
-				cpuidle_get_driver();
-
 			printk(KERN_INFO "Will online and init hotplugged "
 			       "CPU: %d\n", pr->id);
 			WARN(acpi_processor_start(pr), "Failed to start CPU:"
 				" %d\n", pr->id);
 			pr->flags.need_hotplug_init = 0;
-			if (idle_driver && !strcmp(idle_driver->name,
-						   "intel_idle")) {
-				intel_idle_cpu_init(pr->id);
-			}
 		/* Normal CPU soft online event */
 		} else {
 			acpi_processor_ppc_has_changed(pr, 0);
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index d0f59c3..fe95d54 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
 static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
 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;
 
@@ -302,22 +303,35 @@ static void __setup_broadcast_timer(void *arg)
 	clockevents_notify(reason, &cpu);
 }
 
-static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
-		unsigned long action, void *hcpu)
+static int cpu_hotplug_notify(struct notifier_block *n,
+			      unsigned long action, void *hcpu)
 {
 	int hotcpu = (unsigned long)hcpu;
+	struct cpuidle_device *dev;
 
 	switch (action & 0xf) {
 	case CPU_ONLINE:
-		smp_call_function_single(hotcpu, __setup_broadcast_timer,
-			(void *)true, 1);
+
+		if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
+			smp_call_function_single(hotcpu, __setup_broadcast_timer,
+						 (void *)true, 1);
+
+		/*
+		 * Some systems can hotplug a cpu at runtime after
+		 * the kernel has booted, we have to initialize the
+		 * driver in this case
+		 */
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
+		if (!dev->registered)
+			intel_idle_cpu_init(hotcpu);
+
 		break;
 	}
 	return NOTIFY_OK;
 }
 
-static struct notifier_block setup_broadcast_notifier = {
-	.notifier_call = setup_broadcast_cpuhp_notify,
+static struct notifier_block cpu_hotplug_notifier = {
+	.notifier_call = cpu_hotplug_notify,
 };
 
 static void auto_demotion_disable(void *dummy)
@@ -405,10 +419,10 @@ static int intel_idle_probe(void)
 
 	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
 		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
-	else {
+	else
 		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
-		register_cpu_notifier(&setup_broadcast_notifier);
-	}
+
+	register_cpu_notifier(&cpu_hotplug_notifier);
 
 	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
 		" model 0x%X\n", boot_cpu_data.x86_model);
@@ -494,7 +508,7 @@ static int intel_idle_cpuidle_driver_init(void)
  * allocate, initialize, register cpuidle_devices
  * @cpu: cpu/core to initialize
  */
-int intel_idle_cpu_init(int cpu)
+static int intel_idle_cpu_init(int cpu)
 {
 	int cstate;
 	struct cpuidle_device *dev;
@@ -539,7 +553,6 @@ int intel_idle_cpu_init(int cpu)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
 
 static int __init intel_idle_init(void)
 {
@@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void)
 	intel_idle_cpuidle_devices_uninit();
 	cpuidle_unregister_driver(&intel_idle_driver);
 
-	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
+
+	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
 		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
-		unregister_cpu_notifier(&setup_broadcast_notifier);
-	}
+	unregister_cpu_notifier(&cpu_hotplug_notifier);
 
 	return;
 }
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 8684a0d..a6b3f2e 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -207,14 +207,7 @@ struct cpuidle_governor {
 extern int cpuidle_register_governor(struct cpuidle_governor *gov);
 extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
 
-#ifdef CONFIG_INTEL_IDLE
-extern int intel_idle_cpu_init(int cpu);
 #else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
-#endif
-
-#else
-static inline int intel_idle_cpu_init(int cpu) { return -1; }
 
 static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
 {return 0;}
-- 
1.7.5.4

[PATCH 2/4] cpuidle: define the enter function in the driver structure

[Daniel Lezcano]

We have the state index passed as parameter to the 'enter' function.
Most of the drivers assign their 'enter' functions several times in
the cpuidle_state structure, as we have the index, we can delegate
to the driver to handle their own callback array.

That will have the benefit of removing multiple lines of code in the
different drivers.

In order to smoothly modify the driver, the 'enter' function are in
the driver structure and in the cpuidle state structure. That will
let the time to modify the different drivers one by one.
So the 'cpuidle_enter' function checks if the 'enter' callback is
assigned in the driver structure and use it, otherwise it invokes
the 'enter' assigned to the cpuidle_state.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/cpuidle.c |    4 +++-
 include/linux/cpuidle.h   |    1 +
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 0132706..c064144 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -46,7 +46,9 @@ static inline int cpuidle_enter(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int index)
 {
 	struct cpuidle_state *target_state = &drv->states[index];
-	return target_state->enter(dev, drv, index);
+
+	return drv->enter ? drv->enter(dev, drv, index) :
+		target_state->enter(dev, drv, index);
 }
 
 static inline int cpuidle_enter_tk(struct cpuidle_device *dev,
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index a6b3f2e..4d816c7 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -131,6 +131,7 @@ struct cpuidle_driver {
 	struct cpuidle_state	states[CPUIDLE_STATE_MAX];
 	int			state_count;
 	int			safe_state_index;
+	int (*enter)(struct cpuidle_device *, struct cpuidle_driver *, int);
 };
 
 #ifdef CONFIG_CPU_IDLE
-- 
1.7.5.4

[PATCH 3/4] cpuidle: move enter_dead to the driver structure

[Daniel Lezcano]

The 'enter_dead' function is only used for processor_idle.c
and the same function is used several times. We fall into the
same abuse with the multiple callbacks for the same function.

This patch fixes that by moving the 'enter_dead' function to the
driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added
to handle the callback conditional invokation.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/acpi/processor_idle.c |    7 +++----
 drivers/cpuidle/cpuidle.c     |    4 ++--
 include/linux/cpuidle.h       |   26 ++++++++++++++++++++------
 3 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 6d9ec3e..33310fb 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1024,6 +1024,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 struct cpuidle_driver acpi_idle_driver = {
 	.name =		"acpi_idle",
 	.owner =	THIS_MODULE,
+	.enter_dead = acpi_idle_play_dead,
 };
 
 /**
@@ -1132,16 +1133,14 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
 			case ACPI_STATE_C1:
 			if (cx->entry_method == ACPI_CSTATE_FFH)
 				state->flags |= CPUIDLE_FLAG_TIME_VALID;
-
+			state->flags |= CPUIDLE_FLAG_DEAD_VALID;
 			state->enter = acpi_idle_enter_c1;
-			state->enter_dead = acpi_idle_play_dead;
 			drv->safe_state_index = count;
 			break;
 
 			case ACPI_STATE_C2:
-			state->flags |= CPUIDLE_FLAG_TIME_VALID;
+			state->flags |= CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_DEAD_VALID;
 			state->enter = acpi_idle_enter_simple;
-			state->enter_dead = acpi_idle_play_dead;
 			drv->safe_state_index = count;
 			break;
 
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index c064144..7dcfa45 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -81,14 +81,14 @@ int cpuidle_play_dead(void)
 	for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) {
 		struct cpuidle_state *s = &drv->states[i];
 
-		if (s->power_usage < power_usage && s->enter_dead) {
+		if (s->power_usage < power_usage && (s->flags & CPUIDLE_FLAG_DEAD_VALID)) {
 			power_usage = s->power_usage;
 			dead_state = i;
 		}
 	}
 
 	if (dead_state != -1)
-		return drv->states[dead_state].enter_dead(dev, dead_state);
+		return drv->enter_dead(dev, dead_state);
 
 	return -ENODEV;
 }
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 4d816c7..730e12e 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -52,14 +52,25 @@ struct cpuidle_state {
 	int (*enter)	(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,
 			int index);
-
-	int (*enter_dead) (struct cpuidle_device *dev, int index);
 };
 
-/* Idle State Flags */
-#define CPUIDLE_FLAG_TIME_VALID	(0x01) /* is residency time measurable? */
-
-#define CPUIDLE_DRIVER_FLAGS_MASK (0xFFFF0000)
+/********************************************************************************
+ * Idle State Flags:                                                            *
+ * CPUIDLE_FLAG_TIME_VALID  : the drivers could measure the state residency     *
+ *                            time and store it in the 'target_residency'field. *
+ *                            This flag will tell the cpuidle core to use this  *
+ *                            value to compute an accurate prediction.          *
+ *                                                                              *
+ * CPUIDLE_FLAG_COUPLED     : state applies to multiple cpus                    *
+ *                                                                              *
+ * CPUIDLE_FLAG_DEAD_VALID  : the driver may want to deal with dead cpus, tell  *
+ *                            the cpuidle core the specified state can use the  *
+ *                            enter_dead function.                              *
+ *                                                                              *
+ *******************************************************************************/
+#define CPUIDLE_FLAG_TIME_VALID	(0x01)
+#define CPUIDLE_FLAG_COUPLED	(0x02)
+#define CPUIDLE_FLAG_DEAD_VALID (0x04)
 
 /**
  * cpuidle_get_statedata - retrieves private driver state data
@@ -132,6 +143,9 @@ struct cpuidle_driver {
 	int			state_count;
 	int			safe_state_index;
 	int (*enter)(struct cpuidle_device *, struct cpuidle_driver *, int);
+#ifdef CONFIG_ACPI
+	int (*enter_dead)(struct cpuidle_device *, int);
+#endif
 };
 
 #ifdef CONFIG_CPU_IDLE
-- 
1.7.5.4

[PATCH 4/4] cpuidle : move tlb flag to the cpuidle header

[Daniel Lezcano]

Move this specific flag to the header file.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/idle/intel_idle.c |    8 --------
 include/linux/cpuidle.h   |   16 +++++++++++++---
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index fe95d54..3f0eb07 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -101,14 +101,6 @@ 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.
- * If this flag is set, SW flushes the TLB, so even if the
- * HW doesn't do the flushing, this flag is safe to use.
- */
-#define CPUIDLE_FLAG_TLB_FLUSHED	0x10000
-
-/*
  * States are indexed by the cstate number,
  * which is also the index into the MWAIT hint array.
  * Thus C0 is a dummy.
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 730e12e..be150c9 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -67,10 +67,20 @@ struct cpuidle_state {
  *                            the cpuidle core the specified state can use the  *
  *                            enter_dead function.                              *
  *                                                                              *
+ * CPUIDLE_FLAG_TLB_FLUSHED : 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. If this flag is set, SW       *
+ *                            flushes the TLB, so even if the HW doesn't do the *
+ *                            flushing, this flag is safe to use.               *
+ *                                                                              *
  *******************************************************************************/
-#define CPUIDLE_FLAG_TIME_VALID	(0x01)
-#define CPUIDLE_FLAG_COUPLED	(0x02)
-#define CPUIDLE_FLAG_DEAD_VALID (0x04)
+#define CPUIDLE_FLAG_TIME_VALID  (0x01)
+#define CPUIDLE_FLAG_COUPLED     (0x02)
+#define CPUIDLE_FLAG_DEAD_VALID  (0x04)
+#define CPUIDLE_FLAG_TLB_FLUSHED (0x08)
+
+
+
 
 /**
  * cpuidle_get_statedata - retrieves private driver state data
-- 
1.7.5.4

Re: [PATCH 1/4] acpi: intel_idle : break dependency between modules

[Rafael J. Wysocki]

On Thursday, July 05, 2012, Daniel Lezcano wrote:
> When the system is booted with some cpus offline, the idle
> driver is not initialized. When a cpu is set online, the
> acpi code call the intel idle init function. Unfortunately
> this code introduce a dependency between intel_idle and acpi.
> 
> This patch is intended to remove this dependency by using the
> notifier of intel_idle. This patch has the benefit of
> encapsulating the intel_idle driver and remove some exported
> functions.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Acked-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

OK, I'm taking this one.

Thanks,
Rafael


> ---
>  drivers/acpi/processor_driver.c |    7 ------
>  drivers/idle/intel_idle.c       |   41 +++++++++++++++++++++++++-------------
>  include/linux/cpuidle.h         |    7 ------
>  3 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 13103aeb..7048b97 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -429,18 +429,11 @@ static int acpi_cpu_soft_notify(struct notifier_block *nfb,
>  		 * Initialize missing things
>  		 */
>  		if (pr->flags.need_hotplug_init) {
> -			struct cpuidle_driver *idle_driver =
> -				cpuidle_get_driver();
> -
>  			printk(KERN_INFO "Will online and init hotplugged "
>  			       "CPU: %d\n", pr->id);
>  			WARN(acpi_processor_start(pr), "Failed to start CPU:"
>  				" %d\n", pr->id);
>  			pr->flags.need_hotplug_init = 0;
> -			if (idle_driver && !strcmp(idle_driver->name,
> -						   "intel_idle")) {
> -				intel_idle_cpu_init(pr->id);
> -			}
>  		/* Normal CPU soft online event */
>  		} else {
>  			acpi_processor_ppc_has_changed(pr, 0);
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index d0f59c3..fe95d54 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -96,6 +96,7 @@ static const struct idle_cpu *icpu;
>  static struct cpuidle_device __percpu *intel_idle_cpuidle_devices;
>  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;
>  
> @@ -302,22 +303,35 @@ static void __setup_broadcast_timer(void *arg)
>  	clockevents_notify(reason, &cpu);
>  }
>  
> -static int setup_broadcast_cpuhp_notify(struct notifier_block *n,
> -		unsigned long action, void *hcpu)
> +static int cpu_hotplug_notify(struct notifier_block *n,
> +			      unsigned long action, void *hcpu)
>  {
>  	int hotcpu = (unsigned long)hcpu;
> +	struct cpuidle_device *dev;
>  
>  	switch (action & 0xf) {
>  	case CPU_ONLINE:
> -		smp_call_function_single(hotcpu, __setup_broadcast_timer,
> -			(void *)true, 1);
> +
> +		if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
> +			smp_call_function_single(hotcpu, __setup_broadcast_timer,
> +						 (void *)true, 1);
> +
> +		/*
> +		 * Some systems can hotplug a cpu at runtime after
> +		 * the kernel has booted, we have to initialize the
> +		 * driver in this case
> +		 */
> +		dev = per_cpu_ptr(intel_idle_cpuidle_devices, hotcpu);
> +		if (!dev->registered)
> +			intel_idle_cpu_init(hotcpu);
> +
>  		break;
>  	}
>  	return NOTIFY_OK;
>  }
>  
> -static struct notifier_block setup_broadcast_notifier = {
> -	.notifier_call = setup_broadcast_cpuhp_notify,
> +static struct notifier_block cpu_hotplug_notifier = {
> +	.notifier_call = cpu_hotplug_notify,
>  };
>  
>  static void auto_demotion_disable(void *dummy)
> @@ -405,10 +419,10 @@ static int intel_idle_probe(void)
>  
>  	if (boot_cpu_has(X86_FEATURE_ARAT))	/* Always Reliable APIC Timer */
>  		lapic_timer_reliable_states = LAPIC_TIMER_ALWAYS_RELIABLE;
> -	else {
> +	else
>  		on_each_cpu(__setup_broadcast_timer, (void *)true, 1);
> -		register_cpu_notifier(&setup_broadcast_notifier);
> -	}
> +
> +	register_cpu_notifier(&cpu_hotplug_notifier);
>  
>  	pr_debug(PREFIX "v" INTEL_IDLE_VERSION
>  		" model 0x%X\n", boot_cpu_data.x86_model);
> @@ -494,7 +508,7 @@ static int intel_idle_cpuidle_driver_init(void)
>   * allocate, initialize, register cpuidle_devices
>   * @cpu: cpu/core to initialize
>   */
> -int intel_idle_cpu_init(int cpu)
> +static int intel_idle_cpu_init(int cpu)
>  {
>  	int cstate;
>  	struct cpuidle_device *dev;
> @@ -539,7 +553,6 @@ int intel_idle_cpu_init(int cpu)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(intel_idle_cpu_init);
>  
>  static int __init intel_idle_init(void)
>  {
> @@ -581,10 +594,10 @@ static void __exit intel_idle_exit(void)
>  	intel_idle_cpuidle_devices_uninit();
>  	cpuidle_unregister_driver(&intel_idle_driver);
>  
> -	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE) {
> +
> +	if (lapic_timer_reliable_states != LAPIC_TIMER_ALWAYS_RELIABLE)
>  		on_each_cpu(__setup_broadcast_timer, (void *)false, 1);
> -		unregister_cpu_notifier(&setup_broadcast_notifier);
> -	}
> +	unregister_cpu_notifier(&cpu_hotplug_notifier);
>  
>  	return;
>  }
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 8684a0d..a6b3f2e 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -207,14 +207,7 @@ struct cpuidle_governor {
>  extern int cpuidle_register_governor(struct cpuidle_governor *gov);
>  extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
>  
> -#ifdef CONFIG_INTEL_IDLE
> -extern int intel_idle_cpu_init(int cpu);
>  #else
> -static inline int intel_idle_cpu_init(int cpu) { return -1; }
> -#endif
> -
> -#else
> -static inline int intel_idle_cpu_init(int cpu) { return -1; }
>  
>  static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
>  {return 0;}
> 

Re: [PATCH 2/4] cpuidle: define the enter function in the driver structure

[Rafael J. Wysocki]

On Thursday, July 05, 2012, Daniel Lezcano wrote:
> We have the state index passed as parameter to the 'enter' function.
> Most of the drivers assign their 'enter' functions several times in
> the cpuidle_state structure, as we have the index, we can delegate
> to the driver to handle their own callback array.
> 
> That will have the benefit of removing multiple lines of code in the
> different drivers.

Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason.
Among other things, this was to avoid recurrence in callbacks - please see
acpi_idle_enter_bm() for example.

Now, if .enter() is moved to the driver structure, it will have to be an
all-purpose complicated routine calling itself recursively at least in
some cases.  I'm not quite convinced that would be an improvement.

On the other hand, I don't see anything wrong with setting several callback
pointers to the same routine.

Thanks,
Rafael

Re: [PATCH 3/4] cpuidle: move enter_dead to the driver structure

[Rafael J. Wysocki]

On Thursday, July 05, 2012, Daniel Lezcano wrote:
> The 'enter_dead' function is only used for processor_idle.c
> and the same function is used several times. We fall into the
> same abuse with the multiple callbacks for the same function.

This isn't abuse, mind you.  This is a normal practice.

> This patch fixes that by moving the 'enter_dead' function to the
> driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added
> to handle the callback conditional invokation.

And how does that improve things?

Rafael

Re: [PATCH 4/4] cpuidle : move tlb flag to the cpuidle header

[Rafael J. Wysocki]

On Thursday, July 05, 2012, Daniel Lezcano wrote:
> Move this specific flag to the header file.

The patch evidently does more than that.

Is it just a cleanup, or is there a functional reason for doing it?

Rafael


> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/idle/intel_idle.c |    8 --------
>  include/linux/cpuidle.h   |   16 +++++++++++++---
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index fe95d54..3f0eb07 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -101,14 +101,6 @@ 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.
> - * If this flag is set, SW flushes the TLB, so even if the
> - * HW doesn't do the flushing, this flag is safe to use.
> - */
> -#define CPUIDLE_FLAG_TLB_FLUSHED	0x10000
> -
> -/*
>   * States are indexed by the cstate number,
>   * which is also the index into the MWAIT hint array.
>   * Thus C0 is a dummy.
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 730e12e..be150c9 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -67,10 +67,20 @@ struct cpuidle_state {
>   *                            the cpuidle core the specified state can use the  *
>   *                            enter_dead function.                              *
>   *                                                                              *
> + * CPUIDLE_FLAG_TLB_FLUSHED : 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. If this flag is set, SW       *
> + *                            flushes the TLB, so even if the HW doesn't do the *
> + *                            flushing, this flag is safe to use.               *
> + *                                                                              *
>   *******************************************************************************/
> -#define CPUIDLE_FLAG_TIME_VALID	(0x01)
> -#define CPUIDLE_FLAG_COUPLED	(0x02)
> -#define CPUIDLE_FLAG_DEAD_VALID (0x04)
> +#define CPUIDLE_FLAG_TIME_VALID  (0x01)
> +#define CPUIDLE_FLAG_COUPLED     (0x02)
> +#define CPUIDLE_FLAG_DEAD_VALID  (0x04)
> +#define CPUIDLE_FLAG_TLB_FLUSHED (0x08)
> +
> +
> +
>  
>  /**
>   * cpuidle_get_statedata - retrieves private driver state data
> 

Re: [PATCH 2/4] cpuidle: define the enter function in the driver structure

[Daniel Lezcano]

On 07/05/2012 10:38 PM, Rafael J. Wysocki wrote:
> On Thursday, July 05, 2012, Daniel Lezcano wrote:
>> We have the state index passed as parameter to the 'enter' function.
>> Most of the drivers assign their 'enter' functions several times in
>> the cpuidle_state structure, as we have the index, we can delegate
>> to the driver to handle their own callback array.
>>
>> That will have the benefit of removing multiple lines of code in the
>> different drivers.
> 
> Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason.
> Among other things, this was to avoid recurrence in callbacks - please see
> acpi_idle_enter_bm() for example.
> 
> Now, if .enter() is moved to the driver structure, it will have to be an
> all-purpose complicated routine calling itself recursively at least in
> some cases.  I'm not quite convinced that would be an improvement.
> 
> On the other hand, I don't see anything wrong with setting several callback
> pointers to the same routine.

Deepthi sent a few months ago a patch moving the per-cpu cpuidle_state
to a single structure stored in the driver. The drivers were modified
and cleanup to take into account this modification.

We saw some regressions like for example the 'disable' which were not
per cpu and has been moved to the cpuidle_state_usage (and IMHO it is a
workaround more than a real fix). And now we have new architectures
(tegra3, big.LITTLE) with different latencies per cpu. Logically we
should revert Deepthi's patches but from my POV, going back and forth is
not a good solution (also we have to undo all modifications done in the
different drivers).

The main purpose of all these cleanup patches are to move out all
non-data information from the cpuidle_state structure in order to add a
new api which could be 'cpuidle_register_cpu_latency(int cpu, struct
cpuidle_latencies latencies)'.

For this specific patch, the 'enter' function for all the drivers is not
used [1] and one of the driver is about to use a single function [2]. So
we have only one driver is a couple of functions for this which can be
replaced by an array of callbacks in the driver itself as we have the index.

[1] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012355.html
[2] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012399.html


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

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

Re: [PATCH 3/4] cpuidle: move enter_dead to the driver structure

[Daniel Lezcano]

On 07/05/2012 10:40 PM, Rafael J. Wysocki wrote:
> On Thursday, July 05, 2012, Daniel Lezcano wrote:
>> The 'enter_dead' function is only used for processor_idle.c
>> and the same function is used several times. We fall into the
>> same abuse with the multiple callbacks for the same function.
> 
> This isn't abuse, mind you.  This is a normal practice.

Well, that depends :)

I agree adding a callback per state is nice and flexible but if it is
not used, it is a waste of memory, even if it is 32 bytes.

>> This patch fixes that by moving the 'enter_dead' function to the
>> driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added
>> to handle the callback conditional invokation.
> 
> And how does that improve things?

In order to check if the play_dead is enabled for a specific state, we
check if the pointer is set. As it has been moved to a single function,
we need to add a flag to replace this check. That is not an improvement,
it replace a check by another check.


-- 
 <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 4/4] cpuidle : move tlb flag to the cpuidle header

[Daniel Lezcano]

On 07/05/2012 10:43 PM, Rafael J. Wysocki wrote:
> On Thursday, July 05, 2012, Daniel Lezcano wrote:
>> Move this specific flag to the header file.
> 
> The patch evidently does more than that.
> 
> Is it just a cleanup, or is there a functional reason for doing it?

It is just a cleanup.


Thanks for reviewing the patches.

  -- Daniel


>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/idle/intel_idle.c |    8 --------
>>  include/linux/cpuidle.h   |   16 +++++++++++++---
>>  2 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
>> index fe95d54..3f0eb07 100644
>> --- a/drivers/idle/intel_idle.c
>> +++ b/drivers/idle/intel_idle.c
>> @@ -101,14 +101,6 @@ 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.
>> - * If this flag is set, SW flushes the TLB, so even if the
>> - * HW doesn't do the flushing, this flag is safe to use.
>> - */
>> -#define CPUIDLE_FLAG_TLB_FLUSHED	0x10000
>> -
>> -/*
>>   * States are indexed by the cstate number,
>>   * which is also the index into the MWAIT hint array.
>>   * Thus C0 is a dummy.
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index 730e12e..be150c9 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -67,10 +67,20 @@ struct cpuidle_state {
>>   *                            the cpuidle core the specified state can use the  *
>>   *                            enter_dead function.                              *
>>   *                                                                              *
>> + * CPUIDLE_FLAG_TLB_FLUSHED : 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. If this flag is set, SW       *
>> + *                            flushes the TLB, so even if the HW doesn't do the *
>> + *                            flushing, this flag is safe to use.               *
>> + *                                                                              *
>>   *******************************************************************************/
>> -#define CPUIDLE_FLAG_TIME_VALID	(0x01)
>> -#define CPUIDLE_FLAG_COUPLED	(0x02)
>> -#define CPUIDLE_FLAG_DEAD_VALID (0x04)
>> +#define CPUIDLE_FLAG_TIME_VALID  (0x01)
>> +#define CPUIDLE_FLAG_COUPLED     (0x02)
>> +#define CPUIDLE_FLAG_DEAD_VALID  (0x04)
>> +#define CPUIDLE_FLAG_TLB_FLUSHED (0x08)
>> +
>> +
>> +
>>  
>>  /**
>>   * cpuidle_get_statedata - retrieves private driver state data
>>
> 


-- 
 <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/4] cpuidle: define the enter function in the driver structure

[Rafael J. Wysocki]

On Friday, July 06, 2012, Daniel Lezcano wrote:
> On 07/05/2012 10:38 PM, Rafael J. Wysocki wrote:
> > On Thursday, July 05, 2012, Daniel Lezcano wrote:
> >> We have the state index passed as parameter to the 'enter' function.
> >> Most of the drivers assign their 'enter' functions several times in
> >> the cpuidle_state structure, as we have the index, we can delegate
> >> to the driver to handle their own callback array.
> >>
> >> That will have the benefit of removing multiple lines of code in the
> >> different drivers.
> > 
> > Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason.
> > Among other things, this was to avoid recurrence in callbacks - please see
> > acpi_idle_enter_bm() for example.
> > 
> > Now, if .enter() is moved to the driver structure, it will have to be an
> > all-purpose complicated routine calling itself recursively at least in
> > some cases.  I'm not quite convinced that would be an improvement.
> > 
> > On the other hand, I don't see anything wrong with setting several callback
> > pointers to the same routine.
> 
> Deepthi sent a few months ago a patch moving the per-cpu cpuidle_state
> to a single structure stored in the driver. The drivers were modified
> and cleanup to take into account this modification.
> 
> We saw some regressions like for example the 'disable' which were not
> per cpu and has been moved to the cpuidle_state_usage (and IMHO it is a
> workaround more than a real fix). And now we have new architectures
> (tegra3, big.LITTLE) with different latencies per cpu. Logically we
> should revert Deepthi's patches but from my POV, going back and forth is
> not a good solution (also we have to undo all modifications done in the
> different drivers).
> 
> The main purpose of all these cleanup patches are to move out all
> non-data information from the cpuidle_state structure in order to add a
> new api which could be 'cpuidle_register_cpu_latency(int cpu, struct
> cpuidle_latencies latencies)'.

Well, my question is: what specifically is wrong with having callback
pointers in struct cpuidle_state in case when that structure is known
to be common for all the CPUs in the system?

Sure, there are systems in which that is not the case, but then why
should we complicate the simplest case in order to address them?

> For this specific patch, the 'enter' function for all the drivers is not
> used [1] and one of the driver is about to use a single function [2]. So
> we have only one driver is a couple of functions for this which can be
> replaced by an array of callbacks in the driver itself as we have the index.
> 
> [1] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012355.html

I agree with Deepthi.

> [2] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012399.html

And that is fine and dandy as long as the OMAP people want that.  Evidently,
that is not wanted by everybody and if it had been, the cpuidle subsystem
would have been designed differently.

I really would like that thing to be reconsidered before we proceed with
these changes.

Thanks,
Rafael

Re: [PATCH 3/4] cpuidle: move enter_dead to the driver structure

[Rafael J. Wysocki]

On Friday, July 06, 2012, Daniel Lezcano wrote:
> On 07/05/2012 10:40 PM, Rafael J. Wysocki wrote:
> > On Thursday, July 05, 2012, Daniel Lezcano wrote:
> >> The 'enter_dead' function is only used for processor_idle.c
> >> and the same function is used several times. We fall into the
> >> same abuse with the multiple callbacks for the same function.
> > 
> > This isn't abuse, mind you.  This is a normal practice.
> 
> Well, that depends :)
> 
> I agree adding a callback per state is nice and flexible

Yes, it is.

> but if it is not used, it is a waste of memory, even if it is 32 bytes.

32 bits, perhaps?  And how many of those are there in the whole system,
actually?  Is this a number that actually matters?

> >> This patch fixes that by moving the 'enter_dead' function to the
> >> driver structure. A flag CPUIDLE_FLAG_DEAD_VALID has been added
> >> to handle the callback conditional invokation.
> > 
> > And how does that improve things?
> 
> In order to check if the play_dead is enabled for a specific state, we
> check if the pointer is set. As it has been moved to a single function,
> we need to add a flag to replace this check. That is not an improvement,

Well, exactly.

> it replace a check by another check.

Sorry, but I don't really see the point.

Thanks,
Rafael

Re: [PATCH 4/4] cpuidle : move tlb flag to the cpuidle header

[Rafael J. Wysocki]

On Friday, July 06, 2012, Daniel Lezcano wrote:
> On 07/05/2012 10:43 PM, Rafael J. Wysocki wrote:
> > On Thursday, July 05, 2012, Daniel Lezcano wrote:
> >> Move this specific flag to the header file.
> > 
> > The patch evidently does more than that.
> > 
> > Is it just a cleanup, or is there a functional reason for doing it?
> 
> It is just a cleanup.

In that case, can you please say that in the changelog?  I have no objections
to the patch itself, but the changelog is clearly substandard.

> Thanks for reviewing the patches.

No problem.

Thanks,
Rafael

Re: [PATCH 2/4] cpuidle: define the enter function in the driver structure

[Rajendra Nayak]

Hi Daniel,

On Friday 06 July 2012 04:28 PM, Daniel Lezcano wrote:
> The main purpose of all these cleanup patches are to move out all
> non-data information from the cpuidle_state structure in order to add a
> new api which could be 'cpuidle_register_cpu_latency(int cpu, struct
> cpuidle_latencies latencies)'.

So are there any technical difficulties in adding such an api with the
non-data information being part of cpuidle_state? Or its just that you
think the already duplicated non-data information (per state) is going
to be duplicated much more (per state per cpu) in most cases.

regards,
Rajendra

Re: [PATCH 2/4] cpuidle: define the enter function in the driver structure

[Daniel Lezcano]

On 07/10/2012 10:29 AM, Rajendra Nayak wrote:
> Hi Daniel,
> 
> On Friday 06 July 2012 04:28 PM, Daniel Lezcano wrote:
>> The main purpose of all these cleanup patches are to move out all
>> non-data information from the cpuidle_state structure in order to add a
>> new api which could be 'cpuidle_register_cpu_latency(int cpu, struct
>> cpuidle_latencies latencies)'.
> 
> So are there any technical difficulties in adding such an api with the
> non-data information being part of cpuidle_state? Or its just that you
> think the already duplicated non-data information (per state) is going
> to be duplicated much more (per state per cpu) in most cases.

Yes, it is possible. I am already working on an alternate patchset I
will post soon, but I don't like to add more things in a subsystem
before cleaning up the code when it makes sense.

When I looked at the cpuidle code, I noticed this 'enter' field was used
for the same function. With per cpu latency and using the cpuidle_state
structure, that means duplication of this field. For an x86_64
architecture with 16 cores and 4 C-states, that is 512 bytes of memory
instead of 8 bytes.

That is the same for example the 'disable' field which could be stored
in the 'flags' field as a bit instead of the moving it to the 'stats'
structure with "unsigned long long".

As the architectures tend to add more cores [1][2], that means more
memory consumption. In a very near future, we will have 100 cores in a
cpu. Assuming we have 4 C-states, that is 100 * 4 * 8 = 3200 bytes of
memory used for a single function. IMHO, the kernel shouldn't assume the
user must add more memory on the system.

Anyway, I will post a patchset to use the cpuidle_state per cpu.

Thanks
  -- Daniel

[1] http://www.engadget.com/2007/02/11/intel-demonstrates-80-core-processor/
[2] http://www.tilera.com/products/processors/TILE-Gx_Family

-- 
 <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/4] cpuidle: define the enter function in the driver structure

[Rajendra Nayak]

> As the architectures tend to add more cores [1][2], that means more
> memory consumption. In a very near future, we will have 100 cores in a
> cpu. Assuming we have 4 C-states, that is 100 * 4 * 8 = 3200 bytes of
> memory used for a single function. IMHO, the kernel shouldn't assume the
> user must add more memory on the system.

I completely agree.
>
> Anyway, I will post a patchset to use the cpuidle_state per cpu.

Maybe what you were doing wasn't clearly evident with the existing
cpuidle core, but once its scaled to add a per cpu state, it might
look more meaningful. So if you post your patches to add cpuidle
state per cpu, it should certainly give more insight into why this
cleanup makes sense.

>
> Thanks
>    -- Daniel
>
> [1] http://www.engadget.com/2007/02/11/intel-demonstrates-80-core-processor/
> [2] http://www.tilera.com/products/processors/TILE-Gx_Family
>