Messages in this thread | | | Date | Mon, 13 May 2013 14:28:12 +0200 | From | Jean Delvare <> | Subject | Re: [PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration |
| |
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?
> > 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.
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 ;)
-- Jean Delvare
| |