Re: Divide-by-zero in post_init_entity_util_avg

[Andrey Ryabinin <aryabinin@virtuozzo.com>]

On 06/16/2016 03:25 PM, Peter Zijlstra wrote:
> On Thu, Jun 16, 2016 at 10:50:40AM +0200, Peter Zijlstra wrote:
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index f75930bdd326..3fd3d903e6b6 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -2878,6 +2878,20 @@ static inline void cfs_rq_util_change(struct cfs_rq *cfs_rq)
>>  	}
>>  }
>>  
>> +/*
>> + * Explicitly do a load-store to ensure the temporary value never hits memory.
>> + * This allows lockless observations without ever seeing the negative values.
>> + *
>> + * Incidentally, this also generates much saner code for x86.
>> + */
>> +#define sub_positive(type, ptr, val) do {			\
>> +	type tmp = READ_ONCE(*ptr);				\
>> +	tmp -= (val);						\
>> +	if (tmp < 0)						\
>> +		tmp = 0;					\
>> +	WRITE_ONCE(*ptr, tmp);					\
>> +} while (0)
>> +
>>  /* Group cfs_rq's load_avg is used for task_h_load and update_cfs_share */
>>  static inline int
>>  update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq, bool update_freq)
>> @@ -2887,15 +2901,15 @@ update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq, bool update_freq)
>>  
>>  	if (atomic_long_read(&cfs_rq->removed_load_avg)) {
>>  		s64 r = atomic_long_xchg(&cfs_rq->removed_load_avg, 0);
>> -		sa->load_avg = max_t(long, sa->load_avg - r, 0);
>> -		sa->load_sum = max_t(s64, sa->load_sum - r * LOAD_AVG_MAX, 0);
>> +		sub_positive(long, &sa->load_avg, r);
>> +		sub_positive(s64,  &sa->load_sum, r * LOAD_AVG_MAX);
> 
> Hmm, so either we should change these variables to signed types as
> forced here, or this logic (along with the former) is plain wrong.
> 
> As it stands any unsigned value with the MSB set will wipe the field
> after this subtraction.
> 
> I suppose instead we'd want something like:
> 
> 	tmp = READ_ONCE(*ptr);
> 	if (tmp > val)
> 	  tmp -= val;
> 	else
> 	  tmp = 0;
> 	WRITE_ONCE(*ptr, tmp);
> 
> In order to generate:
> 
>   xchg   %rax,0xa0(%r13)
>   mov    0x78(%r13),%rcx
>   sub    %rax,%rcx
>   cmovae %r15,%rcx
>   mov    %rcx,0x78(%r13)
> 
> however, GCC isn't smart enough and generates:
> 
>   xchg   %rax,0x98(%r13)
>   mov    0x70(%r13),%rsi
>   mov    %rsi,%rcx
>   sub    %rax,%rcx
>   cmp    %rsi,%rax
>   cmovae %r15,%rcx
>   mov    %rcx,0x70(%r13)
> 
> Doing a CMP with the _same_ values it does the SUB with, resulting in
> exactly the same CC values.
> 

FYI - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3507  (Reported:	2001-07-01)


> (this is with gcc-5.3, I'm still trying to build gcc-6.1 from the debian
> package which I suppose I should just give up and do a source build)
> 
> Opinions?
>