From patchwork Thu May 5 11:13:10 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Morten Rasmussen X-Patchwork-Id: 67201 Delivered-To: patch@linaro.org Received: by 10.140.92.199 with SMTP id b65csp686425qge; Thu, 5 May 2016 04:12:59 -0700 (PDT) X-Received: by 10.98.72.199 with SMTP id q68mr19693743pfi.164.1462446778996; Thu, 05 May 2016 04:12:58 -0700 (PDT) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id rq15si10878907pab.43.2016.05.05.04.12.58; Thu, 05 May 2016 04:12:58 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753364AbcEELM5 (ORCPT + 29 others); Thu, 5 May 2016 07:12:57 -0400 Received: from foss.arm.com ([217.140.101.70]:49543 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752597AbcEELMz (ORCPT ); Thu, 5 May 2016 07:12:55 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3FA853A; Thu, 5 May 2016 04:13:02 -0700 (PDT) Received: from e105550-lin.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 82AD03F246; Thu, 5 May 2016 04:12:53 -0700 (PDT) Date: Thu, 5 May 2016 12:13:10 +0100 From: Morten Rasmussen To: Yuyang Du Cc: peterz@infradead.org, mingo@kernel.org, linux-kernel@vger.kernel.org, bsegall@google.com, pjt@google.com, vincent.guittot@linaro.org, dietmar.eggemann@arm.com, juri.lelli@arm.com Subject: Re: [PATCH v3 03/12] sched/fair: Change the variable to hold the number of periods to 32bit Message-ID: <20160505111308.GA22310@e105550-lin.cambridge.arm.com> References: <1462305773-7832-1-git-send-email-yuyang.du@intel.com> <1462305773-7832-4-git-send-email-yuyang.du@intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1462305773-7832-4-git-send-email-yuyang.du@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 04, 2016 at 04:02:44AM +0800, Yuyang Du wrote: > In sched average update, a period is about 1ms, so a 32-bit unsigned > integer can approximately hold a maximum of 49 (=2^32/1000/3600/24) > days. > > For usual cases, 32bit is big enough and 64bit is needless. But if > a task sleeps longer than it, there can be two outcomes: > > Consider a task sleeps whatever m milliseconds, then n = (u32)m. Maybe it crystal clear that you mean: For any m > UINT_MAX? > > 1. If n >= 32*64, then the task's sched avgs will be surely decayed > to 0. In this case, it really doesn't matter that the 32bit is not > big enough to hold m. In other words, a task sleeps 2 secs or sleeps > 50 days are the same from sched average point of view. > > 2. If m < 32*64, the chance to be here is very very low, but if so, Should that be: n < 32*64 ? Talking about 32*64, I don't get why we don't use LOAD_AVG_MAX_N. I had a patch ready to post for that: >From 5055e5f82c8d207880035c2ec4ecf1ac1e7f9e91 Mon Sep 17 00:00:00 2001 From: Morten Rasmussen Date: Mon, 11 Apr 2016 15:41:37 +0100 Subject: [PATCH] sched/fair: Fix decay to zero period in decay_load() In __compute_runnable_contrib() we are happy with returning LOAD_AVG_MAX when the update period n >= LOAD_AVG_MAX_N (=345), so we should be happy with returning zero for n >= LOAD_AVG_MAX_N when decaying in decay_load() as well instead of only returning zero for n > LOAD_AVG_PERIOD * 63 (=2016). As both conditions are unlikely() the impact is quite likely very small, but at least it makes the rounding errors for load accumulation and decay symmetrical. Signed-off-by: Morten Rasmussen --- kernel/sched/fair.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 1.9.1 > the task's sched avgs MAY NOT be decayed to 0, depending on how > big its sums are, and the chance to 0 is still good as load_sum > is way less than ~0ULL and util_sum way less than ~0U. I don't get the last bit about load_sum < ~0ULL and util_sum < ~0U. Whether you get to zero depends on the sums (as you say) and the actual value of 'n'. It is true that you might get to zero even if n < LOAD_AVG_MAX_N if the sums are small. > Nevertheless, what really maters is what happens in the worst-case > scenario, which is when (u32)m = 0? So in that case, it would be like > after so long a sleep, we treat the task as it never slept, and has the > same sched averages as before it slept. That is actually not bad or > nothing to worry about, and think of it as the same as how we treat > a new born task. There is subtle but important difference between not decaying a task correctly and adding new task: The sleeping task is already accounted for in the cfs_rq.avg.{load,util}_sum. The sleeping task's contribution to cfs_rq.avg has been decayed correctly in the mean time which means that by not guaranteeing a decay of the se.avg at wake-up you introduce a discrepancy between the task's owen view of its contribution (se.avg) and the cfs_rq view (cfs_rq.avg). That might lead to trouble if the task is migrated at wake-up as we remove se.avg amount of contribution from the previous cpu's cfs_rq.avg. In other words, you remove too much from the cfs_rq.avg. The discrepancy will decay and disappear after LOAD_AVG_MAX_N ms, which might be acceptable, but it is not a totally harmless change IMHO. diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 56b7d4b..42515b6 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -2527,7 +2527,7 @@ static __always_inline u64 decay_load(u64 val, u64 n) if (!n) return val; - else if (unlikely(n > LOAD_AVG_PERIOD * 63)) + else if (unlikely(n > LOAD_AVG_MAX_N)) return 0; /* after bounds checking we can collapse to 32-bit */