linux-kernel - Re: [PATCH v2 1/3] sched/fair: Clean up attach_entity_load

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160531091430.GS3192@twins.programming.kicks-ass.net>
Date:	Tue, 31 May 2016 11:14:30 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Yuyang Du <yuyang.du@...el.com>
Cc:	mingo@...nel.org, linux-kernel@...r.kernel.org, bsegall@...gle.com,
	pjt@...gle.com, morten.rasmussen@....com,
	vincent.guittot@...aro.org, dietmar.eggemann@....com
Subject: Re: [PATCH v2 1/3] sched/fair: Clean up attach_entity_load_avg()

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