Date: Fri, 29 Sep 2017 18:39:02 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Morten Rasmussen <morten.rasmussen@arm.com>
Cc: mingo@kernel.org, linux-kernel@vger.kernel.org, tj@kernel.org,
        josef@toxicpanda.com, torvalds@linux-foundation.org,
        vincent.guittot@linaro.org, efault@gmx.de, pjt@google.com, clm@fb.com,
        dietmar.eggemann@arm.com, bsegall@google.com, yuyang.du@intel.com
Subject: Re: [PATCH -v2 04/18] sched/fair: Remove se->load.weight from
 se->avg.load_sum
Message-ID: <20170929163902.rze5slwzrpr4c2g6@hirez.programming.kicks-ass.net>
References: <20170901132059.342024223@infradead.org>
 <20170901132748.190668510@infradead.org>
 <20170929152607.GC16286@e105550-lin.cambridge.arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170929152607.GC16286@e105550-lin.cambridge.arm.com>
User-Agent: NeoMutt/20170609 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2090
Lines: 60

On Fri, Sep 29, 2017 at 04:26:10PM +0100, Morten Rasmussen wrote:
> On Fri, Sep 01, 2017 at 03:21:03PM +0200, Peter Zijlstra wrote:
> > +/*
> > + * sched_entity:
> > + *
> > + *   load_sum := runnable_sum
> > + *   load_avg = se_weight(se) * runnable_avg
> > + *
> > + * cfq_rs:
> 
> I think this should be "cfs_rq" instead.

Quite.

> > + *
> > + *   load_sum = \Sum se_weight(se) * se->avg.load_sum
> > + *   load_avg = \Sum se->avg.load_avg
> > + */
> 
> I find it a bit confusing that load_sum and load_avg have different
> definitions, but I guess I will discover why dropping weight from
> se->avg.load_sum helps a bit later.

It makes changing the weight cheaper, we don't have to divide the old
weight out and then multiple by the new weight.

Up until this patch set, we mixed the old and new weight in the sum,
with the result that a weight change takes a while to propagate.

But we want to change weight instantly. See:

  sched/fair: More accurate reweight_entity()

> We can't do the same for cfs_rq as it is a \Sum of sums where we
> add/remove contributions when tasks migrate.

Just so. The patch:

  sched/fair: Rewrite PELT migration propagation

Has text on this:

+ * Because while for entities historical weight is not important and we
+ * really only care about our future and therefore can consider a pure
+ * runnable sum, runqueues can NOT do this.
+ *
+ * We specifically want runqueues to have a load_avg that includes
+ * historical weights. Those represent the blocked load, the load we expect
+ * to (shortly) return to us. This only works by keeping the weights as
+ * integral part of the sum. We therefore cannot decompose as per (3).

> Have we defined the relation between runnable_sum and runnable_avg in a
> comment somewhere already? Otherwise it might be helpful to add. It is
> in the code of course :-)

I think this comment here is about it... But who knows. Please keep this
as a note while you read the rest of the series. If you find its still
insufficiently addressed at the end, I'm sure we can do another patch
with moar comments still ;-)