From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756999AbcEaJOi (ORCPT ); Tue, 31 May 2016 05:14:38 -0400 Received: from merlin.infradead.org ([205.233.59.134]:48036 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756118AbcEaJOg (ORCPT ); Tue, 31 May 2016 05:14:36 -0400 Date: Tue, 31 May 2016 11:14:30 +0200 From: Peter Zijlstra To: Yuyang Du Cc: mingo@kernel.org, linux-kernel@vger.kernel.org, bsegall@google.com, pjt@google.com, morten.rasmussen@arm.com, vincent.guittot@linaro.org, dietmar.eggemann@arm.com Subject: Re: [PATCH v2 1/3] sched/fair: Clean up attach_entity_load_avg() Message-ID: <20160531091430.GS3192@twins.programming.kicks-ass.net> References: <1464647577-23057-1-git-send-email-yuyang.du@intel.com> <1464647577-23057-2-git-send-email-yuyang.du@intel.com> <20160531083402.GI3193@twins.programming.kicks-ass.net> <20160531004954.GP18670@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160531004954.GP18670@intel.com> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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..