Re: [PATCH 1/3] sched: Add new API wake_up_if_idle() to wake up the idle cpu

Re: [PATCH 1/3] sched: Add new API wake_up_if_idle() to wake up the idle cpu

[Daniel Lezcano]

On 09/04/2014 09:17 AM, Chuansheng Liu wrote:
> Implementing one new API wake_up_if_idle(), which is used to
> wake up the idle CPU.
>
> Suggested-by: Andy Lutomirski <luto@amacapital.net>
> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>

Hi Chuanseng,

in the future, please prefix the patches with the version number and a 
changelog.

Thanks

   -- Daniel

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

> ---
>   include/linux/sched.h |    1 +
>   kernel/sched/core.c   |   19 +++++++++++++++++++
>   2 files changed, 20 insertions(+)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 857ba40..3f89ac1 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1024,6 +1024,7 @@ struct sched_domain_topology_level {
>   extern struct sched_domain_topology_level *sched_domain_topology;
>
>   extern void set_sched_topology(struct sched_domain_topology_level *tl);
> +extern void wake_up_if_idle(int cpu);
>
>   #ifdef CONFIG_SCHED_DEBUG
>   # define SD_INIT_NAME(type)		.name = #type
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 1211575..b818548 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1620,6 +1620,25 @@ static void ttwu_queue_remote(struct task_struct *p, int cpu)
>   	}
>   }
>
> +void wake_up_if_idle(int cpu)
> +{
> +	struct rq *rq = cpu_rq(cpu);
> +	unsigned long flags;
> +
> +	if (!is_idle_task(rq->curr))
> +		return;
> +
> +	if (set_nr_if_polling(rq->idle)) {
> +		trace_sched_wake_idle_without_ipi(cpu);
> +	} else {
> +		raw_spin_lock_irqsave(&rq->lock, flags);
> +		if (is_idle_task(rq->curr))
> +			smp_send_reschedule(cpu);
> +		/* Else cpu is not in idle, do nothing here */
> +		raw_spin_unlock_irqrestore(&rq->lock, flags);
> +	}
> +}
> +
>   bool cpus_share_cache(int this_cpu, int that_cpu)
>   {
>   	return per_cpu(sd_llc_id, this_cpu) == per_cpu(sd_llc_id, that_cpu);
>


-- 
  <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 1/3] sched: Add new API wake_up_if_idle() to wake up the idle cpu

[Liu, Chuansheng]

> -----Original Message-----
> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> Sent: Thursday, September 04, 2014 8:53 PM
> To: Liu, Chuansheng; peterz@infradead.org; luto@amacapital.net;
> rjw@rjwysocki.net; mingo@redhat.com
> Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Liu, Changcheng;
> Wang, Xiaoming; Chakravarty, Souvik K
> Subject: Re: [PATCH 1/3] sched: Add new API wake_up_if_idle() to wake up the
> idle cpu
> 
> On 09/04/2014 09:17 AM, Chuansheng Liu wrote:
> > Implementing one new API wake_up_if_idle(), which is used to
> > wake up the idle CPU.
> >
> > Suggested-by: Andy Lutomirski <luto@amacapital.net>
> > Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
> 
> Hi Chuanseng,
> 
> in the future, please prefix the patches with the version number and a
> changelog.
> 
Thanks your advice, Daniel, will do that next time:)

Re: [PATCH 2/3] smp: Adding new function wake_up_all_idle_cpus()

[Daniel Lezcano]

On 09/04/2014 09:17 AM, Chuansheng Liu wrote:
> Currently kick_all_cpus_sync() can break non-polling idle cpus
> thru IPI interrupts.
>
> But sometimes we need to break the polling idle cpus immediately
> to reselect the suitable c-state, also for non-idle cpus, we need
> to do nothing if we try to wake up them.
>
> Here adding one new function wake_up_all_idle_cpus() to let all cpus
> out of idle based on function wake_up_if_idle().

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

> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
> ---
>   include/linux/smp.h |    2 ++
>   kernel/smp.c        |   22 ++++++++++++++++++++++
>   2 files changed, 24 insertions(+)
>
> diff --git a/include/linux/smp.h b/include/linux/smp.h
> index 34347f2..93dff5f 100644
> --- a/include/linux/smp.h
> +++ b/include/linux/smp.h
> @@ -100,6 +100,7 @@ int smp_call_function_any(const struct cpumask *mask,
>   			  smp_call_func_t func, void *info, int wait);
>
>   void kick_all_cpus_sync(void);
> +void wake_up_all_idle_cpus(void);
>
>   /*
>    * Generic and arch helpers
> @@ -148,6 +149,7 @@ smp_call_function_any(const struct cpumask *mask, smp_call_func_t func,
>   }
>
>   static inline void kick_all_cpus_sync(void) {  }
> +static inline void wake_up_all_idle_cpus(void) {  }
>
>   #endif /* !SMP */
>
> diff --git a/kernel/smp.c b/kernel/smp.c
> index aff8aa1..9e0d0b2 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -13,6 +13,7 @@
>   #include <linux/gfp.h>
>   #include <linux/smp.h>
>   #include <linux/cpu.h>
> +#include <linux/sched.h>
>
>   #include "smpboot.h"
>
> @@ -699,3 +700,24 @@ void kick_all_cpus_sync(void)
>   	smp_call_function(do_nothing, NULL, 1);
>   }
>   EXPORT_SYMBOL_GPL(kick_all_cpus_sync);
> +
> +/**
> + * wake_up_all_idle_cpus - break all cpus out of idle
> + * wake_up_all_idle_cpus try to break all cpus which is in idle state even
> + * including idle polling cpus, for non-idle cpus, we will do nothing
> + * for them.
> + */
> +void wake_up_all_idle_cpus(void)
> +{
> +	int cpu;
> +
> +	preempt_disable();
> +	for_each_online_cpu(cpu) {
> +		if (cpu == smp_processor_id())
> +			continue;
> +
> +		wake_up_if_idle(cpu);
> +	}
> +	preempt_enable();
> +}
> +EXPORT_SYMBOL_GPL(wake_up_all_idle_cpus);
>


-- 
  <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 3/3] cpuidle: Using the wake_up_all_idle_cpus() to wake up all idle cpus

[Daniel Lezcano]

On 09/04/2014 09:17 AM, Chuansheng Liu wrote:
> Currently kick_all_cpus_sync() or smp_call_function() can not
> break the polling idle cpu immediately.
>
> Here using wake_up_all_idle_cpus() which can wake up the polling idle
> cpu quickly is much helpful for power.

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

So IIUC, kick_all_cpus_sync is a broken function, right ?

Wouldn't make sense to replace all calls to kick_all_cpus by 
wake_up_all_idle_cpus ? and remove this broken function ?

> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
> ---
>   drivers/cpuidle/cpuidle.c |    9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index ee9df5e..d31e04c 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -223,7 +223,7 @@ void cpuidle_uninstall_idle_handler(void)
>   {
>   	if (enabled_devices) {
>   		initialized = 0;
> -		kick_all_cpus_sync();
> +		wake_up_all_idle_cpus();
>   	}
>   }
>
> @@ -530,11 +530,6 @@ EXPORT_SYMBOL_GPL(cpuidle_register);
>
>   #ifdef CONFIG_SMP
>
> -static void smp_callback(void *v)
> -{
> -	/* we already woke the CPU up, nothing more to do */
> -}
> -
>   /*
>    * This function gets called when a part of the kernel has a new latency
>    * requirement.  This means we need to get all processors out of their C-state,
> @@ -544,7 +539,7 @@ static void smp_callback(void *v)
>   static int cpuidle_latency_notify(struct notifier_block *b,
>   		unsigned long l, void *v)
>   {
> -	smp_call_function(smp_callback, NULL, 1);
> +	wake_up_all_idle_cpus();
>   	return NOTIFY_OK;
>   }
>
>


-- 
  <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 3/3] cpuidle: Using the wake_up_all_idle_cpus() to wake up all idle cpus

[Liu, Chuansheng]

> -----Original Message-----
> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> Sent: Thursday, September 04, 2014 9:02 PM
> To: Liu, Chuansheng; peterz@infradead.org; luto@amacapital.net;
> rjw@rjwysocki.net; mingo@redhat.com
> Cc: linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Liu, Changcheng;
> Wang, Xiaoming; Chakravarty, Souvik K
> Subject: Re: [PATCH 3/3] cpuidle: Using the wake_up_all_idle_cpus() to wake up
> all idle cpus
> 
> On 09/04/2014 09:17 AM, Chuansheng Liu wrote:
> > Currently kick_all_cpus_sync() or smp_call_function() can not
> > break the polling idle cpu immediately.
> >
> > Here using wake_up_all_idle_cpus() which can wake up the polling idle
> > cpu quickly is much helpful for power.
> 
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> So IIUC, kick_all_cpus_sync is a broken function, right ?
> 
> Wouldn't make sense to replace all calls to kick_all_cpus by
> wake_up_all_idle_cpus ? and remove this broken function ?
My initial patch(V1) indeed do it, but Andy pointed out some callers of kick_all_cpus_sync()
really want the old behavior.

My fault again that do not have the detailed changelog for the patches.

Pasted the comments from Andy:
==
> Currently using smp_call_function() just woke up the corresponding
> cpu, but can not break the polling idle loop.
>
> Here using the new sched API wake_up_if_idle() to implement it.

kick_all_cpus_sync has other callers, and those other callers want the
old behavior.  I think this should be a new function.

--Andy
==

Re: [PATCH 3/3] cpuidle: Using the wake_up_all_idle_cpus() to wake up all idle cpus

[Peter Zijlstra]

On Thu, Sep 04, 2014 at 01:39:38PM +0000, Liu, Chuansheng wrote:
> > So IIUC, kick_all_cpus_sync is a broken function, right ?

> kick_all_cpus_sync has other callers, and those other callers want the
> old behavior.  I think this should be a new function.

Correct, things like arch/powerpc/mm/pgtable_64.c:pmdp_clear_flush()
really want the old behaviour. It basically uses
local_irq_disable()/local_irq_enable() vs kick_all_cpus_sync() as a RCU
like serialization primitive.