[PATCH V2 1/2] sched: idle: Encapsulate the code to compile it out

[PATCH V2 1/2] sched: idle: Encapsulate the code to compile it out

[Daniel Lezcano]

Encapsulate the large portion of cpuidle_idle_call inside another
function so when CONFIG_CPU_IDLE=n, the code will be compiled out.
Also that is benefitial for the clarity of the code as it removes
a nested indentation level.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 kernel/sched/idle.c |  161 +++++++++++++++++++++++++++------------------------
 1 file changed, 86 insertions(+), 75 deletions(-)

diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index b8cd302..f2f4bc9 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -63,6 +63,90 @@ void __weak arch_cpu_idle(void)
 	local_irq_enable();
 }
 
+#ifdef CONFIG_CPU_IDLE
+static int __cpuidle_idle_call(void)
+{
+	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
+	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+	int next_state, entered_state, ret;
+	bool broadcast;
+
+	/*
+	 * Check if the cpuidle framework is ready, otherwise fallback
+	 * to the default arch specific idle method
+	 */
+	ret = cpuidle_enabled(drv, dev);
+	if (ret)
+		return ret;
+
+	/*
+	 * Ask the governor to choose an idle state it thinks
+	 * it is convenient to go to. There is *always* a
+	 * convenient idle state
+	 */
+	next_state = cpuidle_select(drv, dev);
+
+	/*
+	 * The idle task must be scheduled, it is pointless to
+	 * go to idle, just update no idle residency and get
+	 * out of this function
+	 */
+	if (current_clr_polling_and_test()) {
+		dev->last_residency = 0;
+		entered_state = next_state;
+		local_irq_enable();
+	} else {
+		broadcast = !!(drv->states[next_state].flags &
+			       CPUIDLE_FLAG_TIMER_STOP);
+
+		if (broadcast)
+			/*
+			 * Tell the time framework to switch to a
+			 * broadcast timer because our local timer
+			 * will be shutdown. If a local timer is used
+			 * from another cpu as a broadcast timer, this
+			 * call may fail if it is not available
+			 */
+			ret = clockevents_notify(
+				CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
+				&dev->cpu);
+
+		if (!ret) {
+			trace_cpu_idle_rcuidle(next_state, dev->cpu);
+
+			/*
+			 * Enter the idle state previously returned by
+			 * the governor decision. This function will
+			 * block until an interrupt occurs and will
+			 * take care of re-enabling the local
+			 * interrupts
+			 */
+			entered_state = cpuidle_enter(drv, dev, next_state);
+
+			trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
+
+			if (broadcast)
+				clockevents_notify(
+					CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
+					&dev->cpu);
+
+			/*
+			 * Give the governor an opportunity to reflect
+			 * on the outcome
+			 */
+			cpuidle_reflect(dev, entered_state);
+		}
+	}
+
+	return 0;
+}
+#else
+static int inline __cpuidle_idle_call(void)
+{
+	return -ENOSYS;
+}
+#endif
+
 /**
  * cpuidle_idle_call - the main idle function
  *
@@ -70,10 +154,7 @@ void __weak arch_cpu_idle(void)
  */
 static void cpuidle_idle_call(void)
 {
-	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
-	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
-	int next_state, entered_state, ret;
-	bool broadcast;
+	int ret;
 
 	/*
 	 * Check if the idle task must be rescheduled. If it is the
@@ -100,80 +181,10 @@ static void cpuidle_idle_call(void)
 	rcu_idle_enter();
 
 	/*
-	 * Check if the cpuidle framework is ready, otherwise fallback
-	 * to the default arch specific idle method
-	 */
-	ret = cpuidle_enabled(drv, dev);
-
-	if (!ret) {
-		/*
-		 * Ask the governor to choose an idle state it thinks
-		 * it is convenient to go to. There is *always* a
-		 * convenient idle state
-		 */
-		next_state = cpuidle_select(drv, dev);
-
-		/*
-		 * The idle task must be scheduled, it is pointless to
-		 * go to idle, just update no idle residency and get
-		 * out of this function
-		 */
-		if (current_clr_polling_and_test()) {
-			dev->last_residency = 0;
-			entered_state = next_state;
-			local_irq_enable();
-		} else {
-			broadcast = !!(drv->states[next_state].flags &
-				       CPUIDLE_FLAG_TIMER_STOP);
-
-			if (broadcast)
-				/*
-				 * Tell the time framework to switch
-				 * to a broadcast timer because our
-				 * local timer will be shutdown. If a
-				 * local timer is used from another
-				 * cpu as a broadcast timer, this call
-				 * may fail if it is not available
-				 */
-				ret = clockevents_notify(
-					CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
-					&dev->cpu);
-
-			if (!ret) {
-				trace_cpu_idle_rcuidle(next_state, dev->cpu);
-
-				/*
-				 * Enter the idle state previously
-				 * returned by the governor
-				 * decision. This function will block
-				 * until an interrupt occurs and will
-				 * take care of re-enabling the local
-				 * interrupts
-				 */
-				entered_state = cpuidle_enter(drv, dev,
-							      next_state);
-
-				trace_cpu_idle_rcuidle(PWR_EVENT_EXIT,
-						       dev->cpu);
-
-				if (broadcast)
-					clockevents_notify(
-						CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
-						&dev->cpu);
-
-				/*
-				 * Give the governor an opportunity to reflect on the
-				 * outcome
-				 */
-				cpuidle_reflect(dev, entered_state);
-			}
-		}
-	}
-
-	/*
 	 * We can't use the cpuidle framework, let's use the default
 	 * idle routine
 	 */
+	ret = __cpuidle_idle_call();
 	if (ret)
 		arch_cpu_idle();
 
-- 
1.7.9.5

[PATCH V2 2/2] sched: idle: Store the idle state the cpu is

[Daniel Lezcano]

When the cpu enters idle it stores the cpuidle state pointer in the struct
rq which in turn could be used to take a right decision when balancing
a task.

As soon as the cpu exits the idle state, the structure is filled back with the
NULL pointer.

There are a couple of situations where the idle state pointer could be changed
while looking at it:

1. For x86/acpi with dynamic c-states, when a laptop switches from battery
   to AC that could result on removing the deeper idle state. The acpi driver
   triggers:
	'acpi_processor_cst_has_changed'
		'cpuidle_pause_and_lock'
			'cpuidle_uninstall_idle_handler'
				'kick_all_cpus_sync'.

All cpus will exit their idle state and the pointed object will be set to NULL.

2. The cpuidle driver is unloaded. Logically that could happen but not in
practice because the drivers are always compiled in and 95% of the drivers are
not coded to unregister the driver. In any case, the unloading code must call
'cpuidle_unregister_device', that calls 'cpuidle_pause_and_lock' leading to
'kick_all_cpus_sync' as mentioned above.

The race can happen if we use the pointer and then one of these two situations
occurs at the same moment.

In order to be safe, the idle state pointer stored in the rq must be used inside
a rcu_read_lock section where we are protected with the 'rcu_barrier' in the
'cpuidle_uninstall_idle_handler' function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/cpuidle.c |    6 ++++++
 kernel/sched/idle.c       |    7 +++++++
 kernel/sched/sched.h      |    5 +++++
 3 files changed, 18 insertions(+)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 8236746..6a13f40 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -190,6 +190,12 @@ void cpuidle_install_idle_handler(void)
  */
 void cpuidle_uninstall_idle_handler(void)
 {
+	/*
+	 * Wait for the scheduler to finish to use the idle state he
+	 * may pointing to when looking for the cpu idle states
+	 */
+	rcu_barrier();
+
 	if (enabled_devices) {
 		initialized = 0;
 		kick_all_cpus_sync();
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index f2f4bc9..09029f6 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -12,6 +12,8 @@
 
 #include <trace/events/power.h>
 
+#include "sched.h"
+
 static int __read_mostly cpu_idle_force_poll;
 
 void cpu_idle_poll_ctrl(bool enable)
@@ -66,6 +68,7 @@ void __weak arch_cpu_idle(void)
 #ifdef CONFIG_CPU_IDLE
 static int __cpuidle_idle_call(void)
 {
+	struct rq *rq = this_rq();
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int next_state, entered_state, ret;
@@ -114,6 +117,8 @@ static int __cpuidle_idle_call(void)
 		if (!ret) {
 			trace_cpu_idle_rcuidle(next_state, dev->cpu);
 
+			rq->idle_state = &drv->states[next_state];
+
 			/*
 			 * Enter the idle state previously returned by
 			 * the governor decision. This function will
@@ -123,6 +128,8 @@ static int __cpuidle_idle_call(void)
 			 */
 			entered_state = cpuidle_enter(drv, dev, next_state);
 
+			rq->idle_state = NULL;
+
 			trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
 
 			if (broadcast)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 456e492..bace64a 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -14,6 +14,7 @@
 #include "cpuacct.h"
 
 struct rq;
+struct cpuidle_state;
 
 extern __read_mostly int scheduler_running;
 
@@ -643,6 +644,10 @@ struct rq {
 #ifdef CONFIG_SMP
 	struct llist_head wake_list;
 #endif
+
+#ifdef CONFIG_CPU_IDLE
+	struct cpuidle_state *idle_state;
+#endif
 };
 
 static inline int cpu_of(struct rq *rq)
-- 
1.7.9.5

Re: [PATCH V2 1/2] sched: idle: Encapsulate the code to compile it out

[Rafael J. Wysocki]

On Wednesday, April 30, 2014 02:01:02 PM Daniel Lezcano wrote:
> Encapsulate the large portion of cpuidle_idle_call inside another
> function so when CONFIG_CPU_IDLE=n, the code will be compiled out.
> Also that is benefitial for the clarity of the code as it removes
> a nested indentation level.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Well, this conflicts with

https://patchwork.kernel.org/patch/4071541/

which you haven't commented on and I still want cpuidle_select() to be able to
return negative values because of

https://patchwork.kernel.org/patch/4089631/

(and I have one more patch on top of these two that requires this).

Any ideas how to resolve that?


> ---
>  kernel/sched/idle.c |  161 +++++++++++++++++++++++++++------------------------
>  1 file changed, 86 insertions(+), 75 deletions(-)
> 
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index b8cd302..f2f4bc9 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -63,6 +63,90 @@ void __weak arch_cpu_idle(void)
>  	local_irq_enable();
>  }
>  
> +#ifdef CONFIG_CPU_IDLE
> +static int __cpuidle_idle_call(void)
> +{
> +	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
> +	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
> +	int next_state, entered_state, ret;
> +	bool broadcast;
> +
> +	/*
> +	 * Check if the cpuidle framework is ready, otherwise fallback
> +	 * to the default arch specific idle method
> +	 */
> +	ret = cpuidle_enabled(drv, dev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Ask the governor to choose an idle state it thinks
> +	 * it is convenient to go to. There is *always* a
> +	 * convenient idle state
> +	 */
> +	next_state = cpuidle_select(drv, dev);
> +
> +	/*
> +	 * The idle task must be scheduled, it is pointless to
> +	 * go to idle, just update no idle residency and get
> +	 * out of this function
> +	 */
> +	if (current_clr_polling_and_test()) {
> +		dev->last_residency = 0;
> +		entered_state = next_state;
> +		local_irq_enable();
> +	} else {
> +		broadcast = !!(drv->states[next_state].flags &
> +			       CPUIDLE_FLAG_TIMER_STOP);
> +
> +		if (broadcast)
> +			/*
> +			 * Tell the time framework to switch to a
> +			 * broadcast timer because our local timer
> +			 * will be shutdown. If a local timer is used
> +			 * from another cpu as a broadcast timer, this
> +			 * call may fail if it is not available
> +			 */
> +			ret = clockevents_notify(
> +				CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
> +				&dev->cpu);
> +
> +		if (!ret) {
> +			trace_cpu_idle_rcuidle(next_state, dev->cpu);
> +
> +			/*
> +			 * Enter the idle state previously returned by
> +			 * the governor decision. This function will
> +			 * block until an interrupt occurs and will
> +			 * take care of re-enabling the local
> +			 * interrupts
> +			 */
> +			entered_state = cpuidle_enter(drv, dev, next_state);
> +
> +			trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> +
> +			if (broadcast)
> +				clockevents_notify(
> +					CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
> +					&dev->cpu);
> +
> +			/*
> +			 * Give the governor an opportunity to reflect
> +			 * on the outcome
> +			 */
> +			cpuidle_reflect(dev, entered_state);
> +		}
> +	}
> +
> +	return 0;
> +}
> +#else
> +static int inline __cpuidle_idle_call(void)
> +{
> +	return -ENOSYS;
> +}
> +#endif
> +
>  /**
>   * cpuidle_idle_call - the main idle function
>   *
> @@ -70,10 +154,7 @@ void __weak arch_cpu_idle(void)
>   */
>  static void cpuidle_idle_call(void)
>  {
> -	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
> -	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
> -	int next_state, entered_state, ret;
> -	bool broadcast;
> +	int ret;
>  
>  	/*
>  	 * Check if the idle task must be rescheduled. If it is the
> @@ -100,80 +181,10 @@ static void cpuidle_idle_call(void)
>  	rcu_idle_enter();
>  
>  	/*
> -	 * Check if the cpuidle framework is ready, otherwise fallback
> -	 * to the default arch specific idle method
> -	 */
> -	ret = cpuidle_enabled(drv, dev);
> -
> -	if (!ret) {
> -		/*
> -		 * Ask the governor to choose an idle state it thinks
> -		 * it is convenient to go to. There is *always* a
> -		 * convenient idle state
> -		 */
> -		next_state = cpuidle_select(drv, dev);
> -
> -		/*
> -		 * The idle task must be scheduled, it is pointless to
> -		 * go to idle, just update no idle residency and get
> -		 * out of this function
> -		 */
> -		if (current_clr_polling_and_test()) {
> -			dev->last_residency = 0;
> -			entered_state = next_state;
> -			local_irq_enable();
> -		} else {
> -			broadcast = !!(drv->states[next_state].flags &
> -				       CPUIDLE_FLAG_TIMER_STOP);
> -
> -			if (broadcast)
> -				/*
> -				 * Tell the time framework to switch
> -				 * to a broadcast timer because our
> -				 * local timer will be shutdown. If a
> -				 * local timer is used from another
> -				 * cpu as a broadcast timer, this call
> -				 * may fail if it is not available
> -				 */
> -				ret = clockevents_notify(
> -					CLOCK_EVT_NOTIFY_BROADCAST_ENTER,
> -					&dev->cpu);
> -
> -			if (!ret) {
> -				trace_cpu_idle_rcuidle(next_state, dev->cpu);
> -
> -				/*
> -				 * Enter the idle state previously
> -				 * returned by the governor
> -				 * decision. This function will block
> -				 * until an interrupt occurs and will
> -				 * take care of re-enabling the local
> -				 * interrupts
> -				 */
> -				entered_state = cpuidle_enter(drv, dev,
> -							      next_state);
> -
> -				trace_cpu_idle_rcuidle(PWR_EVENT_EXIT,
> -						       dev->cpu);
> -
> -				if (broadcast)
> -					clockevents_notify(
> -						CLOCK_EVT_NOTIFY_BROADCAST_EXIT,
> -						&dev->cpu);
> -
> -				/*
> -				 * Give the governor an opportunity to reflect on the
> -				 * outcome
> -				 */
> -				cpuidle_reflect(dev, entered_state);
> -			}
> -		}
> -	}
> -
> -	/*
>  	 * We can't use the cpuidle framework, let's use the default
>  	 * idle routine
>  	 */
> +	ret = __cpuidle_idle_call();
>  	if (ret)
>  		arch_cpu_idle();
>  
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 1/2] sched: idle: Encapsulate the code to compile it out

[Rafael J. Wysocki]

On Thursday, May 01, 2014 12:47:25 AM Rafael J. Wysocki wrote:
> On Wednesday, April 30, 2014 02:01:02 PM Daniel Lezcano wrote:
> > Encapsulate the large portion of cpuidle_idle_call inside another
> > function so when CONFIG_CPU_IDLE=n, the code will be compiled out.
> > Also that is benefitial for the clarity of the code as it removes
> > a nested indentation level.
> > 
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Well, this conflicts with
> 
> https://patchwork.kernel.org/patch/4071541/
> 
> which you haven't commented on and I still want cpuidle_select() to be able to
> return negative values because of
> 
> https://patchwork.kernel.org/patch/4089631/
> 
> (and I have one more patch on top of these two that requires this).

Moreover (along the lines of Nico said) after https://patchwork.kernel.org/patch/4071541/
we actually don't need the #ifdef CONFIG_CPU_IDLE in your patch, because cpuidle_select()
for CONFIG_CPU_IDLE unset is a static inline returning a negative number and the compiler
should optimize out the blocks that depend on it being non-negative.

Thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 1/2] sched: idle: Encapsulate the code to compile it out

[Daniel Lezcano]

On 05/01/2014 12:47 AM, Rafael J. Wysocki wrote:
> On Wednesday, April 30, 2014 02:01:02 PM Daniel Lezcano wrote:
>> Encapsulate the large portion of cpuidle_idle_call inside another
>> function so when CONFIG_CPU_IDLE=n, the code will be compiled out.
>> Also that is benefitial for the clarity of the code as it removes
>> a nested indentation level.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>
> Well, this conflicts with
>
> https://patchwork.kernel.org/patch/4071541/
>
> which you haven't commented on and I still want cpuidle_select() to be able to
> return negative values because of
>
> https://patchwork.kernel.org/patch/4089631/
>
> (and I have one more patch on top of these two that requires this).
>
> Any ideas how to resolve that?

I don't think we have a big conflict. If Peter takes your patches before 
than mines then I will refresh and resend them.

I am open to any other suggestion.


Thanks
   -- Daniel


-- 
  <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 V2 1/2] sched: idle: Encapsulate the code to compile it out

[Daniel Lezcano]

On 05/01/2014 12:56 AM, Rafael J. Wysocki wrote:
> On Thursday, May 01, 2014 12:47:25 AM Rafael J. Wysocki wrote:
>> On Wednesday, April 30, 2014 02:01:02 PM Daniel Lezcano wrote:
>>> Encapsulate the large portion of cpuidle_idle_call inside another
>>> function so when CONFIG_CPU_IDLE=n, the code will be compiled out.
>>> Also that is benefitial for the clarity of the code as it removes
>>> a nested indentation level.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>
>> Well, this conflicts with
>>
>> https://patchwork.kernel.org/patch/4071541/
>>
>> which you haven't commented on and I still want cpuidle_select() to be able to
>> return negative values because of
>>
>> https://patchwork.kernel.org/patch/4089631/
>>
>> (and I have one more patch on top of these two that requires this).
>
> Moreover (along the lines of Nico said) after https://patchwork.kernel.org/patch/4071541/
> we actually don't need the #ifdef CONFIG_CPU_IDLE in your patch, because cpuidle_select()
> for CONFIG_CPU_IDLE unset is a static inline returning a negative number and the compiler
> should optimize out the blocks that depend on it being non-negative.

Thanks for the head up.

Actually that was to solve a compilation issue with the next patch when 
adding the cpuidle state in the struct rq.

When the option CPU_IDLE is not set, the code assinging the cpu idle 
state in the rq is still there while in the struct rq the field is 
compiled out with the ifdef macro. If I rely on the compiler 
optimization, the compilation error will happen.


-- 
  <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 V2 1/2] sched: idle: Encapsulate the code to compile it out

[Rafael J. Wysocki]

On Friday, May 02, 2014 10:59:11 AM Daniel Lezcano wrote:
> On 05/01/2014 12:56 AM, Rafael J. Wysocki wrote:
> > On Thursday, May 01, 2014 12:47:25 AM Rafael J. Wysocki wrote:
> >> On Wednesday, April 30, 2014 02:01:02 PM Daniel Lezcano wrote:
> >>> Encapsulate the large portion of cpuidle_idle_call inside another
> >>> function so when CONFIG_CPU_IDLE=n, the code will be compiled out.
> >>> Also that is benefitial for the clarity of the code as it removes
> >>> a nested indentation level.
> >>>
> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>
> >> Well, this conflicts with
> >>
> >> https://patchwork.kernel.org/patch/4071541/
> >>
> >> which you haven't commented on and I still want cpuidle_select() to be able to
> >> return negative values because of
> >>
> >> https://patchwork.kernel.org/patch/4089631/
> >>
> >> (and I have one more patch on top of these two that requires this).
> >
> > Moreover (along the lines of Nico said) after https://patchwork.kernel.org/patch/4071541/
> > we actually don't need the #ifdef CONFIG_CPU_IDLE in your patch, because cpuidle_select()
> > for CONFIG_CPU_IDLE unset is a static inline returning a negative number and the compiler
> > should optimize out the blocks that depend on it being non-negative.
> 
> Thanks for the head up.
> 
> Actually that was to solve a compilation issue with the next patch when 
> adding the cpuidle state in the struct rq.
> 
> When the option CPU_IDLE is not set, the code assinging the cpu idle 
> state in the rq is still there while in the struct rq the field is 
> compiled out with the ifdef macro. If I rely on the compiler 
> optimization, the compilation error will happen.

I see.

If you don't put the new idle_state field in struct_rq under the #ifdef,
you won't need to worry about the build problem.

Alternatively, you can define

#ifdef CONFIG_CPU_IDLE
static inline void rq_set_idle_state(struct rq *rq, struct cpuidle_state *state)
{
	rq->idle_state = state;
}
#else
static inline void rq_set_idle_state(struct rq *rq, struct cpuidle_state *state) {}
#endif

and use rq_set_idle_state() to set that field.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 1/2] sched: idle: Encapsulate the code to compile it out

[Rafael J. Wysocki]

On Friday, May 02, 2014 10:52:27 AM Daniel Lezcano wrote:
> On 05/01/2014 12:47 AM, Rafael J. Wysocki wrote:
> > On Wednesday, April 30, 2014 02:01:02 PM Daniel Lezcano wrote:
> >> Encapsulate the large portion of cpuidle_idle_call inside another
> >> function so when CONFIG_CPU_IDLE=n, the code will be compiled out.
> >> Also that is benefitial for the clarity of the code as it removes
> >> a nested indentation level.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >
> > Well, this conflicts with
> >
> > https://patchwork.kernel.org/patch/4071541/
> >
> > which you haven't commented on and I still want cpuidle_select() to be able to
> > return negative values because of
> >
> > https://patchwork.kernel.org/patch/4089631/
> >
> > (and I have one more patch on top of these two that requires this).
> >
> > Any ideas how to resolve that?
> 
> I don't think we have a big conflict. If Peter takes your patches before 
> than mines then I will refresh and resend them.

Actually, I was planning the merge them myself, because they are more cpuidle
than the scheduler, but either way would be fine.

> I am open to any other suggestion.

Please see the other message I've just sent. :-)

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 1/2] sched: idle: Encapsulate the code to compile it out

[Daniel Lezcano]

On 05/02/2014 02:09 PM, Rafael J. Wysocki wrote:
> On Friday, May 02, 2014 10:59:11 AM Daniel Lezcano wrote:
>> On 05/01/2014 12:56 AM, Rafael J. Wysocki wrote:
>>> On Thursday, May 01, 2014 12:47:25 AM Rafael J. Wysocki wrote:
>>>> On Wednesday, April 30, 2014 02:01:02 PM Daniel Lezcano wrote:
>>>>> Encapsulate the large portion of cpuidle_idle_call inside another
>>>>> function so when CONFIG_CPU_IDLE=n, the code will be compiled out.
>>>>> Also that is benefitial for the clarity of the code as it removes
>>>>> a nested indentation level.
>>>>>
>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>
>>>> Well, this conflicts with
>>>>
>>>> https://patchwork.kernel.org/patch/4071541/
>>>>
>>>> which you haven't commented on and I still want cpuidle_select() to be able to
>>>> return negative values because of
>>>>
>>>> https://patchwork.kernel.org/patch/4089631/
>>>>
>>>> (and I have one more patch on top of these two that requires this).
>>>
>>> Moreover (along the lines of Nico said) after https://patchwork.kernel.org/patch/4071541/
>>> we actually don't need the #ifdef CONFIG_CPU_IDLE in your patch, because cpuidle_select()
>>> for CONFIG_CPU_IDLE unset is a static inline returning a negative number and the compiler
>>> should optimize out the blocks that depend on it being non-negative.
>>
>> Thanks for the head up.
>>
>> Actually that was to solve a compilation issue with the next patch when
>> adding the cpuidle state in the struct rq.
>>
>> When the option CPU_IDLE is not set, the code assinging the cpu idle
>> state in the rq is still there while in the struct rq the field is
>> compiled out with the ifdef macro. If I rely on the compiler
>> optimization, the compilation error will happen.
>
> I see.
>
> If you don't put the new idle_state field in struct_rq under the #ifdef,
> you won't need to worry about the build problem.
>
> Alternatively, you can define
>
> #ifdef CONFIG_CPU_IDLE
> static inline void rq_set_idle_state(struct rq *rq, struct cpuidle_state *state)
> {
> 	rq->idle_state = state;
> }
> #else
> static inline void rq_set_idle_state(struct rq *rq, struct cpuidle_state *state) {}
> #endif
>
> and use rq_set_idle_state() to set that field.

Thanks, I will look at one or another solution.




-- 
  <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 V2 1/2] sched: idle: Encapsulate the code to compile it out

[Daniel Lezcano]

On 05/02/2014 02:09 PM, Rafael J. Wysocki wrote:
> On Friday, May 02, 2014 10:52:27 AM Daniel Lezcano wrote:
>> On 05/01/2014 12:47 AM, Rafael J. Wysocki wrote:
>>> On Wednesday, April 30, 2014 02:01:02 PM Daniel Lezcano wrote:
>>>> Encapsulate the large portion of cpuidle_idle_call inside another
>>>> function so when CONFIG_CPU_IDLE=n, the code will be compiled out.
>>>> Also that is benefitial for the clarity of the code as it removes
>>>> a nested indentation level.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>
>>> Well, this conflicts with
>>>
>>> https://patchwork.kernel.org/patch/4071541/
>>>
>>> which you haven't commented on and I still want cpuidle_select() to be able to
>>> return negative values because of
>>>
>>> https://patchwork.kernel.org/patch/4089631/
>>>
>>> (and I have one more patch on top of these two that requires this).
>>>
>>> Any ideas how to resolve that?
>>
>> I don't think we have a big conflict. If Peter takes your patches before
>> than mines then I will refresh and resend them.
>
> Actually, I was planning the merge them myself, because they are more cpuidle
> than the scheduler, but either way would be fine.

Well I have some patches for the scheduler which will need these 
modifications. Is it possible to merge them throw a common branch to be 
shared between sched and pm ?

>> I am open to any other suggestion.
>
> Please see the other message I've just sent. :-)
>


-- 
  <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 V2 1/2] sched: idle: Encapsulate the code to compile it out

[Rafael J. Wysocki]

On Friday, May 02, 2014 03:35:23 PM Daniel Lezcano wrote:
> On 05/02/2014 02:09 PM, Rafael J. Wysocki wrote:
> > On Friday, May 02, 2014 10:52:27 AM Daniel Lezcano wrote:
> >> On 05/01/2014 12:47 AM, Rafael J. Wysocki wrote:
> >>> On Wednesday, April 30, 2014 02:01:02 PM Daniel Lezcano wrote:
> >>>> Encapsulate the large portion of cpuidle_idle_call inside another
> >>>> function so when CONFIG_CPU_IDLE=n, the code will be compiled out.
> >>>> Also that is benefitial for the clarity of the code as it removes
> >>>> a nested indentation level.
> >>>>
> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>>
> >>> Well, this conflicts with
> >>>
> >>> https://patchwork.kernel.org/patch/4071541/
> >>>
> >>> which you haven't commented on and I still want cpuidle_select() to be able to
> >>> return negative values because of
> >>>
> >>> https://patchwork.kernel.org/patch/4089631/
> >>>
> >>> (and I have one more patch on top of these two that requires this).
> >>>
> >>> Any ideas how to resolve that?
> >>
> >> I don't think we have a big conflict. If Peter takes your patches before
> >> than mines then I will refresh and resend them.
> >
> > Actually, I was planning the merge them myself, because they are more cpuidle
> > than the scheduler, but either way would be fine.
> 
> Well I have some patches for the scheduler which will need these 
> modifications. Is it possible to merge them throw a common branch to be 
> shared between sched and pm ?

That would be perfectly fine by me, but I'm not sure what Ingo and Peter think
about that.

I can set up a branch with sched/idle/cpuidle changes.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 1/2] sched: idle: Encapsulate the code to compile it out

[Nicolas Pitre]

On Fri, 2 May 2014, Rafael J. Wysocki wrote:

> On Friday, May 02, 2014 10:59:11 AM Daniel Lezcano wrote:
> > On 05/01/2014 12:56 AM, Rafael J. Wysocki wrote:
> > > On Thursday, May 01, 2014 12:47:25 AM Rafael J. Wysocki wrote:
> > >> On Wednesday, April 30, 2014 02:01:02 PM Daniel Lezcano wrote:
> > >>> Encapsulate the large portion of cpuidle_idle_call inside another
> > >>> function so when CONFIG_CPU_IDLE=n, the code will be compiled out.
> > >>> Also that is benefitial for the clarity of the code as it removes
> > >>> a nested indentation level.
> > >>>
> > >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > >>
> > >> Well, this conflicts with
> > >>
> > >> https://patchwork.kernel.org/patch/4071541/
> > >>
> > >> which you haven't commented on and I still want cpuidle_select() to be able to
> > >> return negative values because of
> > >>
> > >> https://patchwork.kernel.org/patch/4089631/
> > >>
> > >> (and I have one more patch on top of these two that requires this).
> > >
> > > Moreover (along the lines of Nico said) after https://patchwork.kernel.org/patch/4071541/
> > > we actually don't need the #ifdef CONFIG_CPU_IDLE in your patch, because cpuidle_select()
> > > for CONFIG_CPU_IDLE unset is a static inline returning a negative number and the compiler
> > > should optimize out the blocks that depend on it being non-negative.
> > 
> > Thanks for the head up.
> > 
> > Actually that was to solve a compilation issue with the next patch when 
> > adding the cpuidle state in the struct rq.
> > 
> > When the option CPU_IDLE is not set, the code assinging the cpu idle 
> > state in the rq is still there while in the struct rq the field is 
> > compiled out with the ifdef macro. If I rely on the compiler 
> > optimization, the compilation error will happen.
> 
> I see.
> 
> If you don't put the new idle_state field in struct_rq under the #ifdef,
> you won't need to worry about the build problem.
> 
> Alternatively, you can define
> 
> #ifdef CONFIG_CPU_IDLE
> static inline void rq_set_idle_state(struct rq *rq, struct cpuidle_state *state)
> {
> 	rq->idle_state = state;
> }
> #else
> static inline void rq_set_idle_state(struct rq *rq, struct cpuidle_state *state) {}
> #endif
> 
> and use rq_set_idle_state() to set that field.

Agreed.  And a similar accessor for consumers to get this value in order 
to avoid #ifdef's there as well.


Nicolas

Re: [PATCH V2 1/2] sched: idle: Encapsulate the code to compile it out

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

On Fri, May 02, 2014 at 05:32:25PM +0200, Rafael J. Wysocki wrote:

> That would be perfectly fine by me, but I'm not sure what Ingo and Peter think
> about that.
> 
> I can set up a branch with sched/idle/cpuidle changes.

Ingo typically likes things like that. I'm still a git Luddite, I'll
learn someday :-)

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]