Messages in this thread | | | From | Fredrik Markström <> | Date | Mon, 29 Jun 2015 21:08:34 +0200 | Subject | Re: [PATCH 1/1] cputime: Make the reported utime+stime correspond to the actual runtime. |
| |
I don't think that is good enough. I believe the reason the max()-stuff was initially put there to make sure the returned stime and utime components are increasing monotonically. The scaling code can cause either or to decrease from one call to the other even i rtime increases.
The purpose of my patch is to also make sure that the sum of utime and stime is rtime.
I lost the last part of the patch in my previous email:
- cputime_advance(&prev->stime, stime); - cputime_advance(&prev->utime, utime); + if (stime < prev->stime) { + stime = prev->stime; + utime = rtime - stime; + } else if (utime < prev->utime) { + utime = prev->utime; + stime = rtime - utime; + } -out: + if (prev->stime + prev->utime < rtime) { + prev->stime = stime; + prev->utime = utime; + } *ut = prev->utime; *st = prev->stime;
/Fredrik
On Mon, Jun 29, 2015 at 8:54 PM, Jason Low <jason.low2@hp.com> wrote: > On Mon, 2015-06-29 at 17:28 +0200, Fredrik Markström wrote: >> Hello Peter, the locking part looks good, I don't have a strong >> opinion on per task/signal lock vs global lock. >> >> But with the patch we still update prev->utime and prev->stime >> independently, which was the original problem. But maybe the locking >> and monoticity/sum issue should be addressed by two separate patches >> since they are separate bugs ? >> >> The part I'm referring to is the change below from my original patch >> (maybe without the WARN_ON:s ?): >> >> … >> - cputime_advance(&prev->stime, stime); >> - cputime_advance(&prev->utime, utime); >> + if (stime < prev->stime) { >> + stime = prev->stime; >> + utime = rtime - stime; >> + } else if (utime < prev->utime) { >> + utime = prev->utime; >> + stime = rtime - utime; >> + } >> + WARN_ON(stime < prev->stime); >> + WARN_ON(utime < prev->utime); >> + WARN_ON(stime + utime != rtime); > > How about substituting: > > prev->stime = max(prev->stime, stime); > prev->utime = max(prev->utime, utime); > > with > > if (stime > prev->stime || utime > prev->utime) { > prev->stime = stime; > prev->utime = utime; > } > > in Peter's patch? >
-- /Fredrik
| |