Re: [PATCH 01/11] time: add *_to_jiffies_min helpers to guarantee a minimum duration

[Imre Deak <imre.deak@intel.com>]

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 ;)