Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1409868 > unrolled thread

[PATCH v2 1/3] sched/fair: Clean up attach_entity_load_avg()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 1/3] sched/fair: Clean up attach_entity_load_avg() Yuyang Du <yuyang.du@intel.com> - 2016-05-31 08:40 +0200
    Re: [PATCH v2 1/3] sched/fair: Clean up attach_entity_load_avg() Peter Zijlstra <peterz@infradead.org> - 2016-05-31 10:40 +0200
      Re: [PATCH v2 1/3] sched/fair: Clean up attach_entity_load_avg() Yuyang Du <yuyang.du@intel.com> - 2016-05-31 10:50 +0200
        Re: [PATCH v2 1/3] sched/fair: Clean up attach_entity_load_avg() Peter Zijlstra <peterz@infradead.org> - 2016-05-31 11:20 +0200
      Re: [PATCH v2 1/3] sched/fair: Clean up attach_entity_load_avg() Yuyang Du <yuyang.du@intel.com> - 2016-06-01 04:40 +0200

#1409868 — [PATCH v2 1/3] sched/fair: Clean up attach_entity_load_avg()

attach_entity_load_avg() is called (indirectly) from:

 - switched_to_fair(): switch between classes to fair
 - task_move_group_fair(): move between task groups
 - enqueue_entity_load_avg(): enqueue entity

Only in switched_to_fair() is it possible that the task's last_update_time
is not 0 and therefore the task needs sched avgs update, so move the task
sched avgs update to switched_to_fair() only. In addition, code comments are
clarified.

No functionality change.

Signed-off-by: Yuyang Du <yuyang.du@intel.com>
---
 kernel/sched/fair.c |   44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 218f8e8..a5bdbeb 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2961,24 +2961,6 @@ static inline void update_load_avg(struct sched_entity *se, int update_tg)
 
 static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
-	if (!sched_feat(ATTACH_AGE_LOAD))
-		goto skip_aging;
-
-	/*
-	 * If we got migrated (either between CPUs or between cgroups) we'll
-	 * have aged the average right before clearing @last_update_time.
-	 */
-	if (se->avg.last_update_time) {
-		__update_load_avg(cfs_rq->avg.last_update_time, cpu_of(rq_of(cfs_rq)),
-				  &se->avg, 0, 0, NULL);
-
-		/*
-		 * XXX: we could have just aged the entire load away if we've been
-		 * absent from the fair class for too long.
-		 */
-	}
-
-skip_aging:
 	se->avg.last_update_time = cfs_rq->avg.last_update_time;
 	cfs_rq->avg.load_avg += se->avg.load_avg;
 	cfs_rq->avg.load_sum += se->avg.load_sum;
@@ -8418,6 +8400,27 @@ static void switched_from_fair(struct rq *rq, struct task_struct *p)
 
 static void switched_to_fair(struct rq *rq, struct task_struct *p)
 {
+#ifdef CONFIG_SMP
+	struct sched_entity *se = &p->se;
+
+	if (!sched_feat(ATTACH_AGE_LOAD))
+		goto skip_aging;
+
+	/*
+	 * If we change between classes, age the averages before attaching them.
+	 */
+	if (se->avg.last_update_time) {
+		__update_load_avg(cfs_rq_of(se)->avg.last_update_time, cpu_of(rq),
+				  &se->avg, 0, 0, NULL);
+
+		/*
+		 * XXX: we could have just aged the entire load away if we've been
+		 * absent from the fair class for too long.
+		 */
+	}
+
+skip_aging:
+#endif
 	attach_task_cfs_rq(p);
 
 	if (task_on_rq_queued(p)) {
@@ -8469,11 +8472,6 @@ static void task_move_group_fair(struct task_struct *p)
 {
 	detach_task_cfs_rq(p);
 	set_task_rq(p, task_cpu(p));
-
-#ifdef CONFIG_SMP
-	/* Tell se's cfs_rq has been changed -- migrated */
-	p->se.avg.last_update_time = 0;
-#endif
 	attach_task_cfs_rq(p);
 }
 
-- 
1.7.9.5

[toc] | [next] | [standalone]

#1409992

On Tue, May 31, 2016 at 06:32:54AM +0800, Yuyang Du wrote:
> +++ b/kernel/sched/fair.c
> @@ -2961,24 +2961,6 @@ static inline void update_load_avg(struct sched_entity *se, int update_tg)
>  
>  static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  {
> -	if (!sched_feat(ATTACH_AGE_LOAD))
> -		goto skip_aging;
> -
> -	/*
> -	 * If we got migrated (either between CPUs or between cgroups) we'll
> -	 * have aged the average right before clearing @last_update_time.
> -	 */
> -	if (se->avg.last_update_time) {
> -		__update_load_avg(cfs_rq->avg.last_update_time, cpu_of(rq_of(cfs_rq)),
> -				  &se->avg, 0, 0, NULL);
> -
> -		/*
> -		 * XXX: we could have just aged the entire load away if we've been
> -		 * absent from the fair class for too long.
> -		 */
> -	}
> -
> -skip_aging:
>  	se->avg.last_update_time = cfs_rq->avg.last_update_time;
>  	cfs_rq->avg.load_avg += se->avg.load_avg;
>  	cfs_rq->avg.load_sum += se->avg.load_sum;

So I'm not a big fan of this patch; the aging is a conceptual part of
attaching the load, the fact that it only happens in one callsite is a
mere 'accident'.

The other two patches look reasonable; but I'll wait until Vince has
commented on them..

[toc] | [prev] | [next] | [standalone]

#1409998

On Tue, May 31, 2016 at 10:34:02AM +0200, Peter Zijlstra wrote:
> On Tue, May 31, 2016 at 06:32:54AM +0800, Yuyang Du wrote:
> > +++ b/kernel/sched/fair.c
> > @@ -2961,24 +2961,6 @@ static inline void update_load_avg(struct sched_entity *se, int update_tg)
> >  
> >  static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
> >  {
> > -	if (!sched_feat(ATTACH_AGE_LOAD))
> > -		goto skip_aging;
> > -
> > -	/*
> > -	 * If we got migrated (either between CPUs or between cgroups) we'll
> > -	 * have aged the average right before clearing @last_update_time.
> > -	 */
> > -	if (se->avg.last_update_time) {
> > -		__update_load_avg(cfs_rq->avg.last_update_time, cpu_of(rq_of(cfs_rq)),
> > -				  &se->avg, 0, 0, NULL);
> > -
> > -		/*
> > -		 * XXX: we could have just aged the entire load away if we've been
> > -		 * absent from the fair class for too long.
> > -		 */
> > -	}
> > -
> > -skip_aging:
> >  	se->avg.last_update_time = cfs_rq->avg.last_update_time;
> >  	cfs_rq->avg.load_avg += se->avg.load_avg;
> >  	cfs_rq->avg.load_sum += se->avg.load_sum;
> 
> So I'm not a big fan of this patch; the aging is a conceptual part of
> attaching the load, the fact that it only happens in one callsite is a
> mere 'accident'.

Strictly in concept, it is part of load dealing, maybe not load attaching, :)

[toc] | [prev] | [next] | [standalone]

#1410026

On Tue, May 31, 2016 at 08:49:54AM +0800, Yuyang Du wrote:
> On Tue, May 31, 2016 at 10:34:02AM +0200, Peter Zijlstra wrote:
> > On Tue, May 31, 2016 at 06:32:54AM +0800, Yuyang Du wrote:
> > > +++ b/kernel/sched/fair.c
> > > @@ -2961,24 +2961,6 @@ static inline void update_load_avg(struct sched_entity *se, int update_tg)
> > >  
> > >  static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
> > >  {
> > > -	if (!sched_feat(ATTACH_AGE_LOAD))
> > > -		goto skip_aging;
> > > -
> > > -	/*
> > > -	 * If we got migrated (either between CPUs or between cgroups) we'll
> > > -	 * have aged the average right before clearing @last_update_time.
> > > -	 */
> > > -	if (se->avg.last_update_time) {
> > > -		__update_load_avg(cfs_rq->avg.last_update_time, cpu_of(rq_of(cfs_rq)),
> > > -				  &se->avg, 0, 0, NULL);
> > > -
> > > -		/*
> > > -		 * XXX: we could have just aged the entire load away if we've been
> > > -		 * absent from the fair class for too long.
> > > -		 */
> > > -	}
> > > -
> > > -skip_aging:
> > >  	se->avg.last_update_time = cfs_rq->avg.last_update_time;
> > >  	cfs_rq->avg.load_avg += se->avg.load_avg;
> > >  	cfs_rq->avg.load_sum += se->avg.load_sum;
> > 
> > So I'm not a big fan of this patch; the aging is a conceptual part of
> > attaching the load, the fact that it only happens in one callsite is a
> > mere 'accident'.
> 
> Strictly in concept, it is part of load dealing, maybe not load attaching, :)

Well it deals with the time between detach and attach. Attach is the
obvious point to do that. If you move it elsewhere you run the risk of
forgetting it when the code changes etc..

[toc] | [prev] | [next] | [standalone]

#1410781

On Tue, May 31, 2016 at 10:34:02AM +0200, Peter Zijlstra wrote:
> On Tue, May 31, 2016 at 06:32:54AM +0800, Yuyang Du wrote:
> > +++ b/kernel/sched/fair.c
> > @@ -2961,24 +2961,6 @@ static inline void update_load_avg(struct sched_entity *se, int update_tg)
> >  
> >  static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
> >  {
> > -	if (!sched_feat(ATTACH_AGE_LOAD))
> > -		goto skip_aging;
> > -
> > -	/*
> > -	 * If we got migrated (either between CPUs or between cgroups) we'll
> > -	 * have aged the average right before clearing @last_update_time.
> > -	 */
> > -	if (se->avg.last_update_time) {
> > -		__update_load_avg(cfs_rq->avg.last_update_time, cpu_of(rq_of(cfs_rq)),
> > -				  &se->avg, 0, 0, NULL);
> > -
> > -		/*
> > -		 * XXX: we could have just aged the entire load away if we've been
> > -		 * absent from the fair class for too long.
> > -		 */
> > -	}
> > -
> > -skip_aging:
> >  	se->avg.last_update_time = cfs_rq->avg.last_update_time;
> >  	cfs_rq->avg.load_avg += se->avg.load_avg;
> >  	cfs_rq->avg.load_sum += se->avg.load_sum;
> 
> So I'm not a big fan of this patch; the aging is a conceptual part of
> attaching the load, the fact that it only happens in one callsite is a
> mere 'accident'.

Yes, and we have to put the following to make it an "accident".

static void task_move_group_fair(struct task_struct *p)
{
	detach_task_cfs_rq(p);
	set_task_rq(p, task_cpu(p));

#ifdef CONFIG_SMP
	/* Tell se's cfs_rq has been changed -- migrated */
	p->se.avg.last_update_time = 0;
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#endif
	attach_task_cfs_rq(p);
}

So let me give it another try to keep this cleanup. Will send another version.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web