Date: Sun, 2 Jul 2017 11:37:18 +0200
From: Ingo Molnar <mingo@kernel.org>
To: josef@toxicpanda.com
Cc: mingo@redhat.com, peterz@infradead.org, linux-kernel@vger.kernel.org,
        kernel-team@fb.com, Josef Bacik <jbacik@fb.com>
Subject: Re: [RFC][PATCH] sched: attach extra runtime to the right avg
Message-ID: <20170702093718.aq5p5xxfvrjdeful@gmail.com>
References: <1498787766-9593-1-git-send-email-jbacik@fb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1498787766-9593-1-git-send-email-jbacik@fb.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2650
Lines: 69

* josef@toxicpanda.com <josef@toxicpanda.com> wrote:

> From: Josef Bacik <jbacik@fb.com>
> 
> We only track the load avg of a se in 1024 ns chunks, so in order to
> make up for the loss of the < 1024 ns part of a run/sleep delta we only
> add the time we processed to the se->avg.last_update_time.  The problem
> is there is no way to know if this extra time was while we were asleep
> or while we were running.  Instead keep track of the remainder and apply
> it in the appropriate place.  If the remainder was while we were
> running, add it to the delta the next time we update the load avg while
> running, and the same for sleeping.  This (coupled with other fixes)
> mostly fixes the regression to my workload introduced by Peter's
> experimental runnable load propagation patches.
> 
> Signed-off-by: Josef Bacik <jbacik@fb.com>

> @@ -2897,12 +2904,16 @@ ___update_load_avg(u64 now, int cpu, struct sched_avg *sa,
>  	 * Use 1024ns as the unit of measurement since it's a reasonable
>  	 * approximation of 1us and fast to compute.
>  	 */
> +	remainder = delta & (1023UL);
> +	sa->last_update_time = now;
> +	if (running)
> +		sa->run_remainder = remainder;
> +	else
> +		sa->sleep_remainder = remainder;
>  	delta >>= 10;
>  	if (!delta)
>  		return 0;
>  
> -	sa->last_update_time += delta << 10;
> -

So I'm wondering, this chunk changes how sa->last_update_time is maintained in 
___update_load_avg(): the new code takes a precise timestamp, but the old code was 
not taking an imprecise timestamp, but was updating it via deltas - where each 
delta was rounded down to the nearest 1024 nsecs boundary.

That, if this is the main code path that updates ->last_update_time, creates a 
constant drift of rounding error that skews ->last_update_time into larger and 
larger distances from the real 'now' - ever increasing the value of 'delta'.

An intermediate approach to improve that skew would be something like below. It 
doesn't track the remainder like your patch does, but doesn't lose precision 
either, just rounds down 'now' to the nearest 1024 boundary.

Does this fix the regression you observed as well? Totally untested.

Thanks,

	Ingo

 kernel/sched/fair.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 008c514dc241..b03703cd7989 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2965,7 +2965,7 @@ ___update_load_avg(u64 now, int cpu, struct sched_avg *sa,
 	if (!delta)
 		return 0;
 
-	sa->last_update_time += delta << 10;
+	sa->last_update_time = now & ~1023ULL;
 
 	/*
 	 * Now we know we crossed measurement unit boundaries. The *_avg