Messages in this thread | | | Subject | Re: [PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration | From | Imre Deak <> | Date | Mon, 13 May 2013 16:07:52 +0300 |
| |
On Mon, 2013-05-13 at 14:28 +0200, Jean Delvare wrote: > Hi Imre, > > On Mon, 13 May 2013 14:27:28 +0300, Imre Deak wrote: > > On Mon, 2013-05-13 at 09:29 +0200, Jean Delvare wrote: > > > Hi Imre, > > > > > > On Fri, 10 May 2013 15:13:19 +0300, Imre Deak wrote: > > > > The *_to_jiffies(x) macros return a jiffy value, which if used as a > > > > delta to wait for a specific amount of time, may result in a wait-time > > > > that is less than x. > > > > > > Are you sure? I have always considered that *_to_jiffies(x) macros > > > rounded up, and reading the code seems to confirm that: > > > > > > /* > > > * Generic case - multiply, round and divide. (...) > > > */ > > > (...) > > > return (MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32) > > > >> MSEC_TO_HZ_SHR32; > > > > > > What makes you think the resulting wait time can be less that requested? > > > > Yes the above does a round-up, but for another reason. It makes only > > sure you won't wait less than the requested time because you have a too > > coarse HZ value. So for example with HZ=1000 it won't do any adjustment, > > but with HZ=100 it'll round up durations not dividable by 10 msec. > > For HZ=1000 the code above is never reached, the code which is executed > instead is: > > /* > * HZ is equal to or smaller than 1000, and 1000 is a nice > * round multiple of HZ, divide with the factor between them, > * but round upwards: > */ > return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ); > > which simplifies to just: > > return m; > > So indeed no round up of any kind. Thanks for the clarification. > > > What the proposed change wants to solve is how - or rather what point in > > time - the returned value is used. For example in the following loop to > > wait for some condition to become true: > > > > timeout = msecs_to_jiffies(1); > > while (!condition && timeout) { > > prepare_to_wait(wq, ...); > > timeout = schedule_timeout(timeout); > > } > > > > it would seem we'll wait at least 1 msec for the condition to become > > true. In fact with HZ=1000 and an initial timeout value of 1 we may wait > > less, since schedule_timeout() will return with 0 already at the next > > scheduling clock tick which is most probably less than 1 msec ahead in > > time. > > OK, I see your point now. > > But maybe your example code is not good in the first place. I don't > think you should use schedule_timeout() for such a small wait time. > Aren't you supposed to use HR timers instead?
The problem would be still there even with a longer wait time. msecs_to_jiffies(n) above only guarantees n-1 msecs minimum wait time. This kind of loop - and the wait_for_event_timeout() family of functions where it is embedded - care only about a lower bound to the wait time, and since HR timers are costlier they would only add unneeded overhead here.
> > > If this really is the case then the proper way to address the issue is > > > to fix the original macros, not introducing new ones. > > > > I'm not sure if we need the adjustment in all cases. For example in the > > following polling loop we'd like to wake up every msec (to check for > > something not signaled through the wq) and time out after 50 iterations: > > > > for (i = 0; i < 50; i++) { > > prepare_to_wait(wq, ...); > > if (condition) > > break; > > schedule_timeout(msecs_to_jiffies(1)); > > } > > > > Having the +1 adjustment in msecs_to_jiffies() would result in waking up > > close to every 2 msec. > > To be honest I thought it was already the case, but I was wrong. What > confused me is that I mostly work on hwmon drivers and the typical use > case of msecs_to_jiffies() in these drivers is in conjunction with > time_after(). It's time_after() which does "round up", in that it > always completes the current jiffy before it starts counting.
Right. It's good that you raised this point, it wasn't clear for me either.
--Imre
> So there may be a need for what you're doing, just not in the drivers > I'm taking care of. So I'll keep quiet about it from now on ;)
| |