[PATCH] sched: Get rid of unnecessary checks from select_idle_sibling

[PATCH] sched: Get rid of unnecessary checks from select_idle_sibling

[Namhyung Kim]

From: Namhyung Kim <namhyung.kim@lge.com>

AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
or this_cpu.  So no need to check it again and the conditionals can be
consolidated.

Cc: Mike Galbraith <efault@gmx.de>
Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Alex Shi <alex.shi@intel.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 kernel/sched/fair.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5eea8707234a..af665814c216 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3254,25 +3254,16 @@ find_idlest_cpu(struct sched_group *group, struct task_struct *p, int this_cpu)
  */
 static int select_idle_sibling(struct task_struct *p, int target)
 {
-	int cpu = smp_processor_id();
-	int prev_cpu = task_cpu(p);
 	struct sched_domain *sd;
 	struct sched_group *sg;
 	int i;
 
 	/*
-	 * If the task is going to be woken-up on this cpu and if it is
-	 * already idle, then it is the right target.
-	 */
-	if (target == cpu && idle_cpu(cpu))
-		return cpu;
-
-	/*
-	 * If the task is going to be woken-up on the cpu where it previously
-	 * ran and if it is currently idle, then it the right target.
+	 * If the task is going to be woken-up on this cpu or the cpu where it
+	 * previously ran and it is already idle, then it is the right target.
 	 */
-	if (target == prev_cpu && idle_cpu(prev_cpu))
-		return prev_cpu;
+	if (idle_cpu(target))
+		return target;
 
 	/*
 	 * Otherwise, iterate the domains and find an elegible idle cpu.
-- 
1.7.11.7

Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling

[Alex Shi]

On 01/09/2013 02:50 PM, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
> or this_cpu.  So no need to check it again and the conditionals can be
> consolidated.
> 
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Alex Shi <alex.shi@intel.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  kernel/sched/fair.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 5eea8707234a..af665814c216 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3254,25 +3254,16 @@ find_idlest_cpu(struct sched_group *group, struct task_struct *p, int this_cpu)
>   */
>  static int select_idle_sibling(struct task_struct *p, int target)
>  {
> -	int cpu = smp_processor_id();
> -	int prev_cpu = task_cpu(p);
>  	struct sched_domain *sd;
>  	struct sched_group *sg;
>  	int i;
>  
>  	/*
> -	 * If the task is going to be woken-up on this cpu and if it is
> -	 * already idle, then it is the right target.
> -	 */
> -	if (target == cpu && idle_cpu(cpu))
> -		return cpu;
> -
> -	/*
> -	 * If the task is going to be woken-up on the cpu where it previously
> -	 * ran and if it is currently idle, then it the right target.
> +	 * If the task is going to be woken-up on this cpu or the cpu where it
> +	 * previously ran and it is already idle, then it is the right target.
>  	 */
> -	if (target == prev_cpu && idle_cpu(prev_cpu))
> -		return prev_cpu;
> +	if (idle_cpu(target))
> +		return target;

Uh, we don't know if the target is this_cpu or previous cpu, If we just
check the target idle status, we may miss another idle cpu. So this
patch change the logical in this function.

>  
>  	/*
>  	 * Otherwise, iterate the domains and find an elegible idle cpu.
> 


-- 
Thanks Alex

Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling

[Alex Shi]

> 
> Uh, we don't know if the target is this_cpu or previous cpu, If we just
> check the target idle status, we may miss another idle cpu. So this
> patch change the logical in this function.

But, you can fold wake_affine into select_idle_sibling(). that will save
a complicate calculation whichever cpu idle. :)

-- 
Thanks Alex

Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling

[Namhyung Kim]

On Wed, 09 Jan 2013 15:38:11 +0800, Alex Shi wrote:
>> 
>> Uh, we don't know if the target is this_cpu or previous cpu, If we just
>> check the target idle status, we may miss another idle cpu. So this
>> patch change the logical in this function.
>
> But, you can fold wake_affine into select_idle_sibling(). that will save
> a complicate calculation whichever cpu idle. :)

Looks like a good idea. :)  Will send v2.

Thanks,
Namhyung

Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling

[Namhyung Kim]

Hi Alex,

On Wed, 09 Jan 2013 15:33:40 +0800, Alex Shi wrote:
> On 01/09/2013 02:50 PM, Namhyung Kim wrote:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
>> or this_cpu.  So no need to check it again and the conditionals can be
>> consolidated.
[snip]
> Uh, we don't know if the target is this_cpu or previous cpu, If we just
> check the target idle status, we may miss another idle cpu. So this
> patch change the logical in this function.

select_idle_sibling() is called only in select_task_rq_fair() if it
found a suitable affine_sd.  The default target is the 'prev_cpu' of the
task but if wake_affine() returns true it'd be (this) 'cpu'.

I cannot see where the prev_cpu or the cpu is set to another one before
calling select_idle_sibling.

Thanks,
Namhyung

Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling

[Alex Shi]

On 01/09/2013 03:54 PM, Namhyung Kim wrote:
> Hi Alex,
> 
> On Wed, 09 Jan 2013 15:33:40 +0800, Alex Shi wrote:
>> On 01/09/2013 02:50 PM, Namhyung Kim wrote:
>>> From: Namhyung Kim <namhyung.kim@lge.com>
>>>
>>> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
>>> or this_cpu.  So no need to check it again and the conditionals can be
>>> consolidated.
> [snip]
>> Uh, we don't know if the target is this_cpu or previous cpu, If we just
>> check the target idle status, we may miss another idle cpu. So this
>> patch change the logical in this function.
> 
> select_idle_sibling() is called only in select_task_rq_fair() if it
> found a suitable affine_sd.  The default target is the 'prev_cpu' of the
> task but if wake_affine() returns true it'd be (this) 'cpu'.
> 
> I cannot see where the prev_cpu or the cpu is set to another one before
> calling select_idle_sibling.

The old logical will return directly whenever prev_cpu or this cpu idle,
but your new logical just has one chance.


> 
> Thanks,
> Namhyung
> 


-- 
Thanks Alex

Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling

[Namhyung Kim]

On Wed, 09 Jan 2013 16:34:39 +0800, Alex Shi wrote:
> On 01/09/2013 03:54 PM, Namhyung Kim wrote:
>> Hi Alex,
>> 
>> On Wed, 09 Jan 2013 15:33:40 +0800, Alex Shi wrote:
>>> On 01/09/2013 02:50 PM, Namhyung Kim wrote:
>>>> From: Namhyung Kim <namhyung.kim@lge.com>
>>>>
>>>> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
>>>> or this_cpu.  So no need to check it again and the conditionals can be
>>>> consolidated.
>> [snip]
>>> Uh, we don't know if the target is this_cpu or previous cpu, If we just
>>> check the target idle status, we may miss another idle cpu. So this
>>> patch change the logical in this function.
>> 
>> select_idle_sibling() is called only in select_task_rq_fair() if it
>> found a suitable affine_sd.  The default target is the 'prev_cpu' of the
>> task but if wake_affine() returns true it'd be (this) 'cpu'.
>> 
>> I cannot see where the prev_cpu or the cpu is set to another one before
>> calling select_idle_sibling.
>
> The old logical will return directly whenever prev_cpu or this cpu idle,
> but your new logical just has one chance.

Sorry, I can't get your point.  Could you elaborate on it a bit more?

Thanks,
Namhyung

Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling

[Alex Shi]

On 01/10/2013 01:52 PM, Namhyung Kim wrote:
> On Wed, 09 Jan 2013 16:34:39 +0800, Alex Shi wrote:
>> On 01/09/2013 03:54 PM, Namhyung Kim wrote:
>>> Hi Alex,
>>>
>>> On Wed, 09 Jan 2013 15:33:40 +0800, Alex Shi wrote:
>>>> On 01/09/2013 02:50 PM, Namhyung Kim wrote:
>>>>> From: Namhyung Kim <namhyung.kim@lge.com>
>>>>>
>>>>> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
>>>>> or this_cpu.  So no need to check it again and the conditionals can be
>>>>> consolidated.
>>> [snip]
>>>> Uh, we don't know if the target is this_cpu or previous cpu, If we just
>>>> check the target idle status, we may miss another idle cpu. So this
>>>> patch change the logical in this function.
>>>
>>> select_idle_sibling() is called only in select_task_rq_fair() if it
>>> found a suitable affine_sd.  The default target is the 'prev_cpu' of the
>>> task but if wake_affine() returns true it'd be (this) 'cpu'.
>>>
>>> I cannot see where the prev_cpu or the cpu is set to another one before
>>> calling select_idle_sibling.
>>
>> The old logical will return directly whenever prev_cpu or this cpu idle,
>> but your new logical just has one chance.
> 
> Sorry, I can't get your point.  Could you elaborate on it a bit more?

Sorry. I misunderstand this.

Acked-by: Alex Shi

> 
> Thanks,
> Namhyung
> 


-- 
Thanks Alex

Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling

[Preeti U Murthy]

On 01/09/2013 12:20 PM, Namhyung Kim wrote:
> From: Namhyung Kim <namhyung.kim@lge.com>
> 
> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
> or this_cpu.  So no need to check it again and the conditionals can be
> consolidated.
> 
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Alex Shi <alex.shi@intel.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  kernel/sched/fair.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 5eea8707234a..af665814c216 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3254,25 +3254,16 @@ find_idlest_cpu(struct sched_group *group, struct task_struct *p, int this_cpu)
>   */
>  static int select_idle_sibling(struct task_struct *p, int target)
>  {
> -	int cpu = smp_processor_id();
> -	int prev_cpu = task_cpu(p);
>  	struct sched_domain *sd;
>  	struct sched_group *sg;
>  	int i;
> 
>  	/*
> -	 * If the task is going to be woken-up on this cpu and if it is
> -	 * already idle, then it is the right target.
> -	 */
> -	if (target == cpu && idle_cpu(cpu))
> -		return cpu;
> -
> -	/*
> -	 * If the task is going to be woken-up on the cpu where it previously
> -	 * ran and if it is currently idle, then it the right target.
> +	 * If the task is going to be woken-up on this cpu or the cpu where it
> +	 * previously ran and it is already idle, then it is the right target.
>  	 */
> -	if (target == prev_cpu && idle_cpu(prev_cpu))
> -		return prev_cpu;
> +	if (idle_cpu(target))
> +		return target;
> 
>  	/*
>  	 * Otherwise, iterate the domains and find an elegible idle cpu.
> 
If NUMA_TTWU_BIAS or NUMA_TTWU_TO is true(it is false by
default),cpu/prev_cpu can be changed to be a random node_cpu(the node
that 'this_cpu' is on). In which case even if the node cpu is idle,it
would not be a viable target,looks like.Maybe that is why
select_idle_sibling() makes the check if the target is prev_cpu/this cpu.

Regards
Preeti U Murthy

Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling

[Namhyung Kim]

Hi Preeti,

On Wed, 09 Jan 2013 13:51:00 +0530, Preeti U. Murthy wrote:
> On 01/09/2013 12:20 PM, Namhyung Kim wrote:
>> From: Namhyung Kim <namhyung.kim@lge.com>
>> 
>> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
>> or this_cpu.  So no need to check it again and the conditionals can be
>> consolidated.
[snip]
> If NUMA_TTWU_BIAS or NUMA_TTWU_TO is true(it is false by

I can't find those bits in the code.  I've checked v3.8-rc2,
next-20130110, tip/master and tip/numa/core but there's nothing like
above.  Which tree are you saying?


> default),cpu/prev_cpu can be changed to be a random node_cpu(the node
> that 'this_cpu' is on). In which case even if the node cpu is idle,it
> would not be a viable target,looks like.Maybe that is why
> select_idle_sibling() makes the check if the target is prev_cpu/this cpu.

Looking into tip/numa/core, I can see that there's a code added for
CONFIG_NUMA_BALANCING.  But still, it seems nothing changed on a path
from select_task_rq_fair() to select_idle_sibling() - i.e. if the
select_idle_sibling called, the target would be either prev_cpu or this
cpu.  Am I missing something?

Thanks,
Namhyung

Re: [PATCH] sched: Get rid of unnecessary checks from select_idle_sibling

[Preeti U Murthy]

On 01/10/2013 11:19 AM, Namhyung Kim wrote:
> Hi Preeti,
> 
> On Wed, 09 Jan 2013 13:51:00 +0530, Preeti U. Murthy wrote:
>> On 01/09/2013 12:20 PM, Namhyung Kim wrote:
>>> From: Namhyung Kim <namhyung.kim@lge.com>
>>>
>>> AFAICS @target cpu of select_idle_sibling() is always either prev_cpu
>>> or this_cpu.  So no need to check it again and the conditionals can be
>>> consolidated.
> [snip]
>> If NUMA_TTWU_BIAS or NUMA_TTWU_TO is true(it is false by
> 
> I can't find those bits in the code.  I've checked v3.8-rc2,
> next-20130110, tip/master and tip/numa/core but there's nothing like
> above.  Which tree are you saying?
> 
> 
>> default),cpu/prev_cpu can be changed to be a random node_cpu(the node
>> that 'this_cpu' is on). In which case even if the node cpu is idle,it
>> would not be a viable target,looks like.Maybe that is why
>> select_idle_sibling() makes the check if the target is prev_cpu/this cpu.
> 
> Looking into tip/numa/core, I can see that there's a code added for
> CONFIG_NUMA_BALANCING.  But still, it seems nothing changed on a path
> from select_task_rq_fair() to select_idle_sibling() - i.e. if the
> select_idle_sibling called, the target would be either prev_cpu or this
> cpu.  Am I missing something?

Hi Namhyung,
Sorry about this.I did a git pull on the tip/master.You are right this
piece of code is not there any more.So not an issue.

You can go ahead and add my Reviewed-by.I don't find any potential
problems now.

For your information however, the code piece which was earlier there, is
given below which aimed to pull the tasks towards the node of the waking
cpu,if NUMA_TTWU_BIAS is enabled.

   if (sched_feat_numa(NUMA_TTWU_BIAS) && node != -1) {
               /*
                * For fork,exec find the idlest cpu in the home-node.
                */
               if (sd_flag & (SD_BALANCE_FORK|SD_BALANCE_EXEC)) {
                       int node_cpu = numa_select_node_cpu(p, node);
                       if (node_cpu < 0)
                               goto find_sd;

                       new_cpu = cpu = node_cpu;
                       sd = per_cpu(sd_node, cpu);
                       goto pick_idlest;
               }

               /*
                * For wake, pretend we were running in the home-node.
                */
               if (cpu_to_node(prev_cpu) != node) {
                       int node_cpu = numa_select_node_cpu(p, node);
                       if (node_cpu < 0)
                               goto find_sd;

                       if (sched_feat_numa(NUMA_TTWU_TO))
                               cpu = node_cpu;
                       else
                               prev_cpu = node_cpu;
               }
       }

> 
> Thanks,
> Namhyung
> 
I apologise once again for the wrong feedback.

Thank you

Regards
Preeti U Murthy