Re: [kernel-sched-cputime] question about probable bug in cputime_adjust()

Gustavo A. R. Silva
2017-06-29T17:58:47Z

Quoting Frans Klaver <fransklaver@gmail.com>:

> On 29 June 2017 01:57:19 CEST, "Gustavo A. R. Silva"
> <garsilva@embeddedor.com> wrote:
>>>>> --- a/kernel/sched/cputime.c
>>>>> +++ b/kernel/sched/cputime.c
>>>>> @@ -637,9 +637,10 @@ static void cputime_adjust(struct task_cputime
>> *curr,
>>>>> * = (rtime_i+1 - rtime_i) + utime_i
>>>>> * >= utime_i
>>>>> */
>>>>> - if (stime < prev->stime)
>>>>> + if (stime < prev->stime) {
>>>>> stime = prev->stime;
>>>>> - utime = rtime - stime;
>>>>> + utime = rtime - stime;
>>>>> + }
>>>>>
>>>>>
>>>>> If you confirm this, I will send a patch in a full and proper form.
>>>>>
>>>>> I'd really appreciate your comments.
>>>>
>>>> If you do that, how would you meet the guarantee made in line 583?
>>>>
>>
>> You are right, I see now.
>>
>> Then in this case the following patch would be the way to go:
>>
>> --- a/kernel/sched/cputime.c
>> +++ b/kernel/sched/cputime.c
>> @@ -615,10 +615,8 @@ static void cputime_adjust(struct task_cputime
>> *curr,
>> * userspace. Once a task gets some ticks, the monotonicy code at
>> * 'update' will ensure things converge to the observed ratio.
>> */
>> - if (stime == 0) {
>> - utime = rtime;
>> + if (stime == 0)
>> goto update;
>> - }
>>
>> if (utime == 0) {
>> stime = rtime;
>>
>>
>> but I think this one is even better:
>>
>>
>> --- a/kernel/sched/cputime.c
>> +++ b/kernel/sched/cputime.c
>> @@ -615,19 +615,11 @@ static void cputime_adjust(struct task_cputime
>> *curr,
>> * userspace. Once a task gets some ticks, the monotonicy code at
>> * 'update' will ensure things converge to the observed ratio.
>> */
>> - if (stime == 0) {
>> - utime = rtime;
>> - goto update;
>> - }
>> -
>> - if (utime == 0) {
>> + if (stime != 0 && utime == 0)
>> stime = rtime;
>> - goto update;
>> - }
>> -
>> - stime = scale_stime(stime, rtime, stime + utime);
>> + else
>> + stime = scale_stime(stime, rtime, stime + utime);
>
> I don't think it is better. The stime == 0 case is gone now. So
> scale_time() will be called in that case. This whole if/else block
> should only be executed if stime != 0.

Oh yeah! something like:

if (stime != 0) {
if (stime != 0 && utime == 0)
stime = rtime;
else
stime = scale_stime(stime, rtime, stime + utime);
}

I'll be right back with the final patch.

Thanks for your time, Frans.
Much appreciated :)
--
Gustavo A. R. Silva