[PATCH RFC] timer: drop the unnecessary while loop in msleep

[PATCH RFC] timer: drop the unnecessary while loop in msleep

[Nicholas Mc Guire]

 The while loop in msleep does not seem necessary as
timeout is unsigned long and no larger than MAX_JIFFY_OFFSET (which is
LONG_MAX/2 - 1) so the while-loop condition is always true at the beginning
(msecs_to_jiffies will return >=0 always and with the +1 timeout is >= 1 so
the while condition is always true at the start) and
schedule_timeout_uninterruptible always returns 0, so the while loop always
terminates after the first loop.

So msleep really should be a pure wrapper to
  schedule_timeout_uninterruptible(msecs_to_jiffies(timeout));

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Q: what is the purpose of the + 1 offset to the jiffies here ?

msleep was introduced in 2.6.7 but without the + 1, so with:
	unsigned long timeout = msecs_to_jiffies(msecs);
in 2.6.10-rc2 the msecs_to_jiffies(msecs) + 1; is introduced.
Nishanth Aravamudan <nacc@us.ibm.com> (https://lkml.org/lkml/2004/11/19/294)
seems to be the origin while converting msleep to a macro, but no reason
for the + 1 is given there.

The msecs_to_jiffies(msecs) + 1; is not clear to me what purpose it serves
it also pre-dates git... so I was not able to find the rational for the + 1
in the commit logs (and its not documented in
Documentation/timers/timers-howto.txt either). From 
http://lkml.org/lkml/2007/8/3/250 it though seems clear that msleep should
not be in use for small values of timeout anyway and msleep(0) is just as
bad as msleep(1) with respect to performance.

>From my understanding the + 1 is not necessary and a call to msleep(0)
would be perfectly legal (__mode_timer will handle it correctly) though not
desirable and the + 1 could also be dropped.

patch was compile tested with: x86_64_defconfig

patch is against linux-next 4.4-rc8 (localversion-next is -next-20160108)

 kernel/time/timer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 2c40347..3b131d3 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1685,8 +1685,7 @@ void msleep(unsigned int msecs)
 {
 	unsigned long timeout = msecs_to_jiffies(msecs) + 1;
 
-	while (timeout)
-		timeout = schedule_timeout_uninterruptible(timeout);
+	timeout = schedule_timeout_uninterruptible(timeout);
 }
 
 EXPORT_SYMBOL(msleep);
-- 
2.1.4

Re: [PATCH RFC] timer: drop the unnecessary while loop in msleep

[Thomas Gleixner]

On Sat, 9 Jan 2016, Nicholas Mc Guire wrote:

>  The while loop in msleep does not seem necessary as
> timeout is unsigned long and no larger than MAX_JIFFY_OFFSET (which is
> LONG_MAX/2 - 1) so the while-loop condition is always true at the beginning
> (msecs_to_jiffies will return >=0 always and with the +1 timeout is >= 1 so
> the while condition is always true at the start) and
> schedule_timeout_uninterruptible always returns 0, so the while loop always
> terminates after the first loop.

Err, no. schedule_timeout_uninterruptible() can return > 0 when there was a
non timer wakeup. Thinks spurious wakeups. So we need that loop.
 
> Q: what is the purpose of the + 1 offset to the jiffies here ?
> 
> msleep was introduced in 2.6.7 but without the + 1, so with:
> 	unsigned long timeout = msecs_to_jiffies(msecs);
> in 2.6.10-rc2 the msecs_to_jiffies(msecs) + 1; is introduced.
> Nishanth Aravamudan <nacc@us.ibm.com> (https://lkml.org/lkml/2004/11/19/294)
> seems to be the origin while converting msleep to a macro, but no reason
> for the + 1 is given there.

Not really. The +1 was introduced with the following commit:

https://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/kernel/timer.c?id=c259ef842622a5e64418d9dab3b62ee051867edf

Thanks,

	tglx

Re: [PATCH RFC] timer: drop the unnecessary while loop in msleep

[Nicholas Mc Guire]

On Mon, Jan 11, 2016 at 09:15:25AM +0100, Thomas Gleixner wrote:
> On Sat, 9 Jan 2016, Nicholas Mc Guire wrote:
> 
> >  The while loop in msleep does not seem necessary as
> > timeout is unsigned long and no larger than MAX_JIFFY_OFFSET (which is
> > LONG_MAX/2 - 1) so the while-loop condition is always true at the beginning
> > (msecs_to_jiffies will return >=0 always and with the +1 timeout is >= 1 so
> > the while condition is always true at the start) and
> > schedule_timeout_uninterruptible always returns 0, so the while loop always
> > terminates after the first loop.
> 
> Err, no. schedule_timeout_uninterruptible() can return > 0 when there was a
> non timer wakeup. Thinks spurious wakeups. So we need that loop.

ok - thanks - was following the comment in schedule_timeout which states:
/**
 * schedule_timeout - sleep until timeout
 * @timeout: timeout value in jiffies
 *
 * Make the current task sleep until @timeout jiffies have
 * elapsed. The routine will return immediately unless
 * the current task state has been set (see set_current_state()).
 *
 * You can set the task state as follows -
 *
 * %TASK_UNINTERRUPTIBLE - at least @timeout jiffies are guaranteed to
 * pass before the routine returns. The routine will return 0
<snip>

So I had assumed that would not actualy be possible given this comment.
Is the while(timeout) loop im msleep() just defensive programming or is 
there a spurious timer wakeup path that defeats TASK_UNINTERRUPTIBLE ? 
Probably a stupid question but I was unable to figure out how such an 
wakeup would occure.

>  
> > Q: what is the purpose of the + 1 offset to the jiffies here ?
> > 
> > msleep was introduced in 2.6.7 but without the + 1, so with:
> > 	unsigned long timeout = msecs_to_jiffies(msecs);
> > in 2.6.10-rc2 the msecs_to_jiffies(msecs) + 1; is introduced.
> > Nishanth Aravamudan <nacc@us.ibm.com> (https://lkml.org/lkml/2004/11/19/294)
> > seems to be the origin while converting msleep to a macro, but no reason
> > for the + 1 is given there.
> 
> Not really. The +1 was introduced with the following commit:
> 
> https://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/kernel/timer.c?id=c259ef842622a5e64418d9dab3b62ee051867edf
>

thanks - was ignorant of history.git being available.
will go try and understand where that "lost jiffie" could come from.

thx!
hofrat

Re: [PATCH RFC] timer: drop the unnecessary while loop in msleep

[Thomas Gleixner]

On Tue, 12 Jan 2016, Nicholas Mc Guire wrote:
> On Mon, Jan 11, 2016 at 09:15:25AM +0100, Thomas Gleixner wrote:
> > On Sat, 9 Jan 2016, Nicholas Mc Guire wrote:
> > 
> > >  The while loop in msleep does not seem necessary as
> > > timeout is unsigned long and no larger than MAX_JIFFY_OFFSET (which is
> > > LONG_MAX/2 - 1) so the while-loop condition is always true at the beginning
> > > (msecs_to_jiffies will return >=0 always and with the +1 timeout is >= 1 so
> > > the while condition is always true at the start) and
> > > schedule_timeout_uninterruptible always returns 0, so the while loop always
> > > terminates after the first loop.
> > 
> > Err, no. schedule_timeout_uninterruptible() can return > 0 when there was a
> > non timer wakeup. Thinks spurious wakeups. So we need that loop.
> 
> ok - thanks - was following the comment in schedule_timeout which states:
> /**
>  * schedule_timeout - sleep until timeout
>  * @timeout: timeout value in jiffies
>  *
>  * Make the current task sleep until @timeout jiffies have
>  * elapsed. The routine will return immediately unless
>  * the current task state has been set (see set_current_state()).
>  *
>  * You can set the task state as follows -
>  *
>  * %TASK_UNINTERRUPTIBLE - at least @timeout jiffies are guaranteed to
>  * pass before the routine returns. The routine will return 0
> <snip>
> 
> So I had assumed that would not actualy be possible given this comment.
> Is the while(timeout) loop im msleep() just defensive programming or is 
> there a spurious timer wakeup path that defeats TASK_UNINTERRUPTIBLE ? 
> Probably a stupid question but I was unable to figure out how such an 
> wakeup would occure.

That comment is wrong. Just a simple example:

     wait_for_completion_io_timeout(x, timeout)
       wait_for_common_io(x, timeout, TASK_UNINTERRUPTIBLE)
         __wait_for_common(x, io_schedule_timeout, timeout, state)
	   do_wait_for_common(x, action, timeout, state)
   	     ...
	     io_schedule_timeout(timeout)
		schedule_timeout(timeout);

So the function can return either because the timer fired or the completion
completed. In the latter case the @timeout jiffies certainly have not passed.

> > > Q: what is the purpose of the + 1 offset to the jiffies here ?
> > > 
> > > msleep was introduced in 2.6.7 but without the + 1, so with:
> > > 	unsigned long timeout = msecs_to_jiffies(msecs);
> > > in 2.6.10-rc2 the msecs_to_jiffies(msecs) + 1; is introduced.
> > > Nishanth Aravamudan <nacc@us.ibm.com> (https://lkml.org/lkml/2004/11/19/294)
> > > seems to be the origin while converting msleep to a macro, but no reason
> > > for the + 1 is given there.
> > 
> > Not really. The +1 was introduced with the following commit:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/kernel/timer.c?id=c259ef842622a5e64418d9dab3b62ee051867edf
> >
> 
> thanks - was ignorant of history.git being available.
> will go try and understand where that "lost jiffie" could come from.

Assume HZ=1000 and msleep(1). If the msleep() is issued right at the edge of
the next tick interrupt, then it would return almost immediately. Certainly
not what you would expect, right?

Thanks,

	tglx