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

[Gustavo A. R. Silva]

Quoting Frans Klaver <franskla...@gmail.com>:

On 29 June 2017 01:57:19 CEST, "Gustavo A. R. Silva" <garsi...@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