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

[Peter Zijlstra]

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..