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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <1452337264-32209-1-git-send-email-hofrat@osadl.org>
Date:	Sat,  9 Jan 2016 12:01:04 +0100
From:	Nicholas Mc Guire <hofrat@...dl.org>
To:	John Stultz <john.stultz@...aro.org>
Cc:	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org,
	Nicholas Mc Guire <hofrat@...dl.org>
Subject: [PATCH RFC] timer: drop the unnecessary while loop in msleep

 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@...dl.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@...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