Messages in this thread | | | Date | Thu, 4 Sep 2014 14:30:56 -0700 | Subject | Re: [PATCH] sched: fix timeval conversion to jiffies | From | John Stultz <> |
| |
On Thu, Sep 4, 2014 at 2:17 PM, Andrew Hunter <ahh@google.com> wrote: > On Wed, Sep 3, 2014 at 5:06 PM, John Stultz <john.stultz@linaro.org> wrote: >> Maybe with the next version of the patch, before you get into the >> unwinding the math, you might practically describe what is broken, >> then explain how its broken. >> >> My quick read here is that we're converting a timespec -> jiffies, and >> in doing so we round up by one jiffy. >> >> This seems actually perfectly normal, as we usually end up rounding up >> by a jiffy in many cases since we don't want to undershoot any >> timeout, and we're always allowed return later then specified. > > Well, yes, timeouts can be longer than specified, but what you said > technically applies just as much to code that arbitrarily multiplies > jiffies by 10 before returning, no? :) > > The problem isn't the rounding, it's that the rounding is _wrong_: I'm > fine rounding 10100usec / (1000 usec/jiffie) = 11 jiffies. The current > code rounds 10000usec / (1000 usec/jiffies) to 11. I've rewritten the > description to make this clearer.
Ok. Very much appreciated!
>>> In particular, with HZ=1000, we consistently computed that 10000 usec >>> was 11 jiffies; the same was true for any exact multiple of >>> TICK_NSEC. This is obviously bad as a general rule, and caused >>> observable user problems with setitimer() at the very least: >>> >>> setitimer(ITIMER_PROF, &val, NULL); >>> setitimer(ITIMER_PROF, NULL, &val); >>> >>> would actually add a tick to val! >> >> So this looks like an issue. Since we convert and store the internal >> representation in jiffies, when we pull it back out, we get the >> rounded up value, which is larger then the timespec value originally >> submitted. This is really the core issue, correct? > > For the particular user bug reported to me, yes, this was the core > issue: some code that stopped and restarted an itimer found the > interval growing by 1ms each time. But again, it wasn't that it was > rounded: if we initially passed e.g. 10500 usec and got back 11000, > that'd be annoying but workable, because if we then went through > another cycle of enabling/disabling itimer, we'd set it to 11000 usec > and get back 11000 again. What we have now instead adds a full jiffie > _every time_.
Ah, ok. This part is key to understanding the problem. Thanks for clarifying this.
This seems to be a quite old bug.. Do you think this is needed for -stable?
thanks -john
| |