Re: [PATCH 3/8] sched: don't consider other cpus in our group in case of NEWLY_IDLE

[Joonsoo Kim]

On Tue, Mar 19, 2013 at 03:20:57PM +0100, Peter Zijlstra wrote:
> On Thu, 2013-02-14 at 14:48 +0900, Joonsoo Kim wrote:
> > Commit 88b8dac0 makes load_balance() consider other cpus in its group,
> > regardless of idle type. When we do NEWLY_IDLE balancing, we should not
> > consider it, because a motivation of NEWLY_IDLE balancing is to turn
> > this cpu to non idle state if needed. This is not the case of other cpus.
> > So, change code not to consider other cpus for NEWLY_IDLE balancing.
> > 
> > With this patch, assign 'if (pulled_task) this_rq->idle_stamp = 0'
> > in idle_balance() is corrected, because NEWLY_IDLE balancing doesn't
> > consider other cpus. Assigning to 'this_rq->idle_stamp' is now valid.
> > 
> > Cc: Srivatsa Vaddagiri 
> > Signed-off-by: Joonsoo Kim 
> 
> Fair enough, good catch.
> 
> Acked-by: Peter Zijlstra 
> 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 0c6aaf6..97498f4 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -5016,8 +5016,15 @@ static int load_balance(int this_cpu, struct rq 
> > *this_rq,
> > .cpus   = cpus,
> > };
> >  
> > +   /* For NEWLY_IDLE load_balancing, we don't need to consider
> > +* other cpus in our group */
> > +   if (idle == CPU_NEWLY_IDLE) {
> > +   env.dst_grpmask = NULL;
> > +   max_lb_iterations = 0;
> 
> Just a small nit; I don't think we'll ever get to evaluate
> max_lb_iterations when !dst_grpmask. So strictly speaking its
> superfluous to touch it.

Okay. In next spin, I will remove it and add a comment here.

Thanks.

> 
> > +   } else {
> > +   max_lb_iterations = cpumask_weight(env.dst_grpmask);
> > +   }
> > cpumask_copy(cpus, cpu_active_mask);
> > -   max_lb_iterations = cpumask_weight(env.dst_grpmask);
> >  
> > schedstat_inc(sd, lb_count[idle]);
> >  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/8] sched: don't consider other cpus in our group in case of NEWLY_IDLE

[Joonsoo Kim]

On Tue, Mar 19, 2013 at 03:20:57PM +0100, Peter Zijlstra wrote:
 On Thu, 2013-02-14 at 14:48 +0900, Joonsoo Kim wrote:
  Commit 88b8dac0 makes load_balance() consider other cpus in its group,
  regardless of idle type. When we do NEWLY_IDLE balancing, we should not
  consider it, because a motivation of NEWLY_IDLE balancing is to turn
  this cpu to non idle state if needed. This is not the case of other cpus.
  So, change code not to consider other cpus for NEWLY_IDLE balancing.
  
  With this patch, assign 'if (pulled_task) this_rq-idle_stamp = 0'
  in idle_balance() is corrected, because NEWLY_IDLE balancing doesn't
  consider other cpus. Assigning to 'this_rq-idle_stamp' is now valid.
  
  Cc: Srivatsa Vaddagiri va...@linux.vnet.ibm.com
  Signed-off-by: Joonsoo Kim iamjoonsoo@lge.com
 
 Fair enough, good catch.
 
 Acked-by: Peter Zijlstra a.p.zijls...@chello.nl
 
  diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
  index 0c6aaf6..97498f4 100644
  --- a/kernel/sched/fair.c
  +++ b/kernel/sched/fair.c
  @@ -5016,8 +5016,15 @@ static int load_balance(int this_cpu, struct rq 
  *this_rq,
  .cpus   = cpus,
  };
   
  +   /* For NEWLY_IDLE load_balancing, we don't need to consider
  +* other cpus in our group */
  +   if (idle == CPU_NEWLY_IDLE) {
  +   env.dst_grpmask = NULL;
  +   max_lb_iterations = 0;
 
 Just a small nit; I don't think we'll ever get to evaluate
 max_lb_iterations when !dst_grpmask. So strictly speaking its
 superfluous to touch it.

Okay. In next spin, I will remove it and add a comment here.

Thanks.

 
  +   } else {
  +   max_lb_iterations = cpumask_weight(env.dst_grpmask);
  +   }
  cpumask_copy(cpus, cpu_active_mask);
  -   max_lb_iterations = cpumask_weight(env.dst_grpmask);
   
  schedstat_inc(sd, lb_count[idle]);
   
 
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/8] sched: don't consider other cpus in our group in case of NEWLY_IDLE

[Peter Zijlstra]

On Thu, 2013-02-14 at 14:48 +0900, Joonsoo Kim wrote:
> Commit 88b8dac0 makes load_balance() consider other cpus in its group,
> regardless of idle type. When we do NEWLY_IDLE balancing, we should not
> consider it, because a motivation of NEWLY_IDLE balancing is to turn
> this cpu to non idle state if needed. This is not the case of other cpus.
> So, change code not to consider other cpus for NEWLY_IDLE balancing.
> 
> With this patch, assign 'if (pulled_task) this_rq->idle_stamp = 0'
> in idle_balance() is corrected, because NEWLY_IDLE balancing doesn't
> consider other cpus. Assigning to 'this_rq->idle_stamp' is now valid.
> 
> Cc: Srivatsa Vaddagiri 
> Signed-off-by: Joonsoo Kim 

Fair enough, good catch.

Acked-by: Peter Zijlstra 

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 0c6aaf6..97498f4 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5016,8 +5016,15 @@ static int load_balance(int this_cpu, struct rq 
> *this_rq,
>   .cpus   = cpus,
>   };
>  
> + /* For NEWLY_IDLE load_balancing, we don't need to consider
> +  * other cpus in our group */
> + if (idle == CPU_NEWLY_IDLE) {
> + env.dst_grpmask = NULL;
> + max_lb_iterations = 0;

Just a small nit; I don't think we'll ever get to evaluate
max_lb_iterations when !dst_grpmask. So strictly speaking its
superfluous to touch it.

> + } else {
> + max_lb_iterations = cpumask_weight(env.dst_grpmask);
> + }
>   cpumask_copy(cpus, cpu_active_mask);
> - max_lb_iterations = cpumask_weight(env.dst_grpmask);
>  
>   schedstat_inc(sd, lb_count[idle]);
>  


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/8] sched: don't consider other cpus in our group in case of NEWLY_IDLE

[Peter Zijlstra]

On Thu, 2013-02-14 at 14:48 +0900, Joonsoo Kim wrote:
 Commit 88b8dac0 makes load_balance() consider other cpus in its group,
 regardless of idle type. When we do NEWLY_IDLE balancing, we should not
 consider it, because a motivation of NEWLY_IDLE balancing is to turn
 this cpu to non idle state if needed. This is not the case of other cpus.
 So, change code not to consider other cpus for NEWLY_IDLE balancing.
 
 With this patch, assign 'if (pulled_task) this_rq-idle_stamp = 0'
 in idle_balance() is corrected, because NEWLY_IDLE balancing doesn't
 consider other cpus. Assigning to 'this_rq-idle_stamp' is now valid.
 
 Cc: Srivatsa Vaddagiri va...@linux.vnet.ibm.com
 Signed-off-by: Joonsoo Kim iamjoonsoo@lge.com

Fair enough, good catch.

Acked-by: Peter Zijlstra a.p.zijls...@chello.nl

 diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
 index 0c6aaf6..97498f4 100644
 --- a/kernel/sched/fair.c
 +++ b/kernel/sched/fair.c
 @@ -5016,8 +5016,15 @@ static int load_balance(int this_cpu, struct rq 
 *this_rq,
   .cpus   = cpus,
   };
  
 + /* For NEWLY_IDLE load_balancing, we don't need to consider
 +  * other cpus in our group */
 + if (idle == CPU_NEWLY_IDLE) {
 + env.dst_grpmask = NULL;
 + max_lb_iterations = 0;

Just a small nit; I don't think we'll ever get to evaluate
max_lb_iterations when !dst_grpmask. So strictly speaking its
superfluous to touch it.

 + } else {
 + max_lb_iterations = cpumask_weight(env.dst_grpmask);
 + }
   cpumask_copy(cpus, cpu_active_mask);
 - max_lb_iterations = cpumask_weight(env.dst_grpmask);
  
   schedstat_inc(sd, lb_count[idle]);
  


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/8] sched: don't consider other cpus in our group in case of NEWLY_IDLE

[Joonsoo Kim]

Commit 88b8dac0 makes load_balance() consider other cpus in its group,
regardless of idle type. When we do NEWLY_IDLE balancing, we should not
consider it, because a motivation of NEWLY_IDLE balancing is to turn
this cpu to non idle state if needed. This is not the case of other cpus.
So, change code not to consider other cpus for NEWLY_IDLE balancing.

With this patch, assign 'if (pulled_task) this_rq->idle_stamp = 0'
in idle_balance() is corrected, because NEWLY_IDLE balancing doesn't
consider other cpus. Assigning to 'this_rq->idle_stamp' is now valid.

Cc: Srivatsa Vaddagiri 
Signed-off-by: Joonsoo Kim 

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 0c6aaf6..97498f4 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5016,8 +5016,15 @@ static int load_balance(int this_cpu, struct rq *this_rq,
.cpus   = cpus,
};
 
+   /* For NEWLY_IDLE load_balancing, we don't need to consider
+* other cpus in our group */
+   if (idle == CPU_NEWLY_IDLE) {
+   env.dst_grpmask = NULL;
+   max_lb_iterations = 0;
+   } else {
+   max_lb_iterations = cpumask_weight(env.dst_grpmask);
+   }
cpumask_copy(cpus, cpu_active_mask);
-   max_lb_iterations = cpumask_weight(env.dst_grpmask);
 
schedstat_inc(sd, lb_count[idle]);
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/8] sched: don't consider other cpus in our group in case of NEWLY_IDLE

[Joonsoo Kim]

Commit 88b8dac0 makes load_balance() consider other cpus in its group,
regardless of idle type. When we do NEWLY_IDLE balancing, we should not
consider it, because a motivation of NEWLY_IDLE balancing is to turn
this cpu to non idle state if needed. This is not the case of other cpus.
So, change code not to consider other cpus for NEWLY_IDLE balancing.

With this patch, assign 'if (pulled_task) this_rq-idle_stamp = 0'
in idle_balance() is corrected, because NEWLY_IDLE balancing doesn't
consider other cpus. Assigning to 'this_rq-idle_stamp' is now valid.

Cc: Srivatsa Vaddagiri va...@linux.vnet.ibm.com
Signed-off-by: Joonsoo Kim iamjoonsoo@lge.com

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 0c6aaf6..97498f4 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5016,8 +5016,15 @@ static int load_balance(int this_cpu, struct rq *this_rq,
.cpus   = cpus,
};
 
+   /* For NEWLY_IDLE load_balancing, we don't need to consider
+* other cpus in our group */
+   if (idle == CPU_NEWLY_IDLE) {
+   env.dst_grpmask = NULL;
+   max_lb_iterations = 0;
+   } else {
+   max_lb_iterations = cpumask_weight(env.dst_grpmask);
+   }
cpumask_copy(cpus, cpu_active_mask);
-   max_lb_iterations = cpumask_weight(env.dst_grpmask);
 
schedstat_inc(sd, lb_count[idle]);
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/