From: bsegall@google.com
To: Yuyang Du <yuyang.du@intel.com>
Cc: peterz@infradead.org, mingo@kernel.org, linux-kernel@vger.kernel.org,
        pjt@google.com, morten.rasmussen@arm.com, vincent.guittot@linaro.org,
        dietmar.eggemann@arm.com, juri.lelli@arm.com
Subject: Re: [PATCH 3/6] sched/fair: Change the variable to hold the number of periods to 32bit integer
References: <1461812173-32439-1-git-send-email-yuyang.du@intel.com>
	<1461812173-32439-4-git-send-email-yuyang.du@intel.com>
Date: Thu, 28 Apr 2016 10:29:55 -0700
In-Reply-To: <1461812173-32439-4-git-send-email-yuyang.du@intel.com> (Yuyang
	Du's message of "Thu, 28 Apr 2016 10:56:10 +0800")
Message-ID: <xm26vb317ib0.fsf@bsegall-linux.mtv.corp.google.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3120
Lines: 90

Yuyang Du <yuyang.du@intel.com> writes:

> Now a period is about 1ms, so a 32-bit unsigned integer can approximately
> hold a maximum of 49 (=2^32/1000/3600/24) days, which means it is big enough
> and 64-bit is needless.
>
If a thread sleeps for 49 days and then wakes up this would be wrong...
but it also would just result in it not being decayed to zero, and even
then only if it was in a very small window, so it doesn't seem like a
huge deal if it happens.


> Signed-off-by: Yuyang Du <yuyang.du@intel.com>
> ---
>  kernel/sched/fair.c |   27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8d49276..abfe17a 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -2619,18 +2619,13 @@ static const u32 __accumulated_sum_N32[] = {
>   * n is the number of periods past; a period is ~1ms
>   * m is called half-life in exponential decay; here it is SCHED_AVG_HALFLIFE=32.
>   */
> -static __always_inline u64 __decay_sum(u64 val, u64 n)
> +static __always_inline u64 __decay_sum(u64 val, u32 n)
>  {
> -	unsigned int local_n;
> -
>  	if (!n)
>  		return val;
>  	else if (unlikely(n > SCHED_AVG_HALFLIFE * 63))
>  		return 0;
>  
> -	/* after bounds checking we can collapse to 32-bit */
> -	local_n = n;
> -
>  	/*
>  	 * As y^PERIOD = 1/2, we can combine
>  	 *    y^n = 1/2^(n/PERIOD) * y^(n%PERIOD)
> @@ -2638,12 +2633,12 @@ static __always_inline u64 __decay_sum(u64 val, u64 n)
>  	 *
>  	 * To achieve constant time decay_load.
>  	 */
> -	if (unlikely(local_n >= SCHED_AVG_HALFLIFE)) {
> -		val >>= local_n / SCHED_AVG_HALFLIFE;
> -		local_n %= SCHED_AVG_HALFLIFE;
> +	if (unlikely(n >= SCHED_AVG_HALFLIFE)) {
> +		val >>= n / SCHED_AVG_HALFLIFE;
> +		n %= SCHED_AVG_HALFLIFE;
>  	}
>  
> -	val = mul_u64_u32_shr(val, __decay_inv_multiply_N[local_n], 32);
> +	val = mul_u64_u32_shr(val, __decay_inv_multiply_N[n], 32);
>  	return val;
>  }
>  
> @@ -2654,7 +2649,7 @@ static __always_inline u64 __decay_sum(u64 val, u64 n)
>   * We can compute this efficiently by combining:
>   * y^32 = 1/2 with precomputed \Sum 1024*y^n   (where n < 32)
>   */
> -static u32 __accumulate_sum(u64 n)
> +static u32 __accumulate_sum(u32 n)
>  {
>  	u32 contrib = 0;
>  
> @@ -2708,8 +2703,8 @@ static __always_inline int
>  __update_sched_avg(u64 now, int cpu, struct sched_avg *sa,
>  		  unsigned long weight, int running, struct cfs_rq *cfs_rq)
>  {
> -	u64 delta, scaled_delta, periods;
> -	u32 contrib;
> +	u64 delta, scaled_delta;
> +	u32 contrib, periods;
>  	unsigned int delta_w, scaled_delta_w, decayed = 0;
>  	unsigned long scale_freq, scale_cpu;
>  
> @@ -2762,7 +2757,11 @@ __update_sched_avg(u64 now, int cpu, struct sched_avg *sa,
>  
>  		delta -= delta_w;
>  
> -		/* Figure out how many additional periods this update spans */
> +		/*
> +		 * Figure out how many additional periods this update spans.
> +		 * A period is 1024*1024ns or ~1ms, so a 32bit integer can hold
> +		 * approximately a maximum of 49 (=2^32/1000/3600/24) days.
> +		 */
>  		periods = delta / 1024;
>  		delta %= 1024;