* [PATCH RFC] timer: drop the unnecessary while loop in msleep @ 2016-01-09 11:01 Nicholas Mc Guire 2016-01-11 8:15 ` Thomas Gleixner 0 siblings, 1 reply; 4+ messages in thread From: Nicholas Mc Guire @ 2016-01-09 11:01 UTC (permalink / raw) To: John Stultz; +Cc: Thomas Gleixner, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RFC] timer: drop the unnecessary while loop in msleep 2016-01-09 11:01 [PATCH RFC] timer: drop the unnecessary while loop in msleep Nicholas Mc Guire @ 2016-01-11 8:15 ` Thomas Gleixner 2016-01-12 12:48 ` Nicholas Mc Guire 0 siblings, 1 reply; 4+ messages in thread From: Thomas Gleixner @ 2016-01-11 8:15 UTC (permalink / raw) To: Nicholas Mc Guire; +Cc: John Stultz, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC] timer: drop the unnecessary while loop in msleep 2016-01-11 8:15 ` Thomas Gleixner @ 2016-01-12 12:48 ` Nicholas Mc Guire 2016-01-12 13:53 ` Thomas Gleixner 0 siblings, 1 reply; 4+ messages in thread From: Nicholas Mc Guire @ 2016-01-12 12:48 UTC (permalink / raw) To: Thomas Gleixner; +Cc: Nicholas Mc Guire, John Stultz, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC] timer: drop the unnecessary while loop in msleep 2016-01-12 12:48 ` Nicholas Mc Guire @ 2016-01-12 13:53 ` Thomas Gleixner 0 siblings, 0 replies; 4+ messages in thread From: Thomas Gleixner @ 2016-01-12 13:53 UTC (permalink / raw) To: Nicholas Mc Guire; +Cc: Nicholas Mc Guire, John Stultz, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-01-12 13:54 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-09 11:01 [PATCH RFC] timer: drop the unnecessary while loop in msleep Nicholas Mc Guire 2016-01-11 8:15 ` Thomas Gleixner 2016-01-12 12:48 ` Nicholas Mc Guire 2016-01-12 13:53 ` Thomas Gleixner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.