From: Nishanth Aravamudan <nacc@us.ibm.com>
To: linux-kernel@vger.kernel.org
Cc: domen@coderock.org
Subject: [PATCH 1/2] kernel/timer.c: remove msleep() and msleep_interruptible()
Date: Fri, 19 Nov 2004 16:48:41 -0800 [thread overview]
Message-ID: <20041120004840.GA7466@us.ibm.com> (raw)
In-Reply-To: <20041117024944.GB4218@us.ibm.com>
On Tue, Nov 16, 2004 at 06:49:44PM -0800, Nishanth Aravamudan wrote:
> Hi,
>
> After some pretty heavy discussion on IRC, I felt that it may be
> important / useful to bring the discussion of schedule_timeout() to
> LKML. There are two issues being considered:
>
> 1) msleep_interruptible()
>
> For reference, here is the code for this function:
>
> /**
> * msleep_interruptible - sleep waiting for waitqueue interruptions
> * @msecs: Time in milliseconds to sleep for
> */
> unsigned long msleep_interruptible(unsigned int msecs)
> {
> unsigned long timeout = msecs_to_jiffies(msecs);
>
> while (timeout && !signal_pending(current)) {
> set_current_state(TASK_INTERRUPTIBLE);
> timeout = schedule_timeout(timeout);
> }
> return jiffies_to_msecs(timeout);
> }
>
> The first issue deals with whether the while() loop is at all necessary.
> From my understanding (primarily from how the code "should" behave, but
> also backed up by code itself), I think the following code:
>
> set_current_state(TASK_INTERRUPTIBLE);
> timeout = schedule_timeout(timeout);
>
> should be interpreted as:
>
> a) I wish to sleep for timeout jiffies; however
> b) If a signal occurs before timeout jiffies have gone by, I
> would also like to wake up.
>
> With this interpretation, though, the while()-conditional becomes
> questionable. I can see two cases (think inclusive OR not exclusive) for
> schedule_timeout() returning:
>
> a) A signal was received and thus signal_pending(current) will
> be true, exiting the loop. In this case, timeout will be
> some non-negative value (0 is a corner case, I believe, where
> both the timer fires and a signal is received in the last jiffy).
> b) The timer in schedule_timeout() has expired and thus it will
> return 0. This indicates the function has delayed the requested
> time (at least) and timeout will be set to 0, again exiting the
> loop.
>
> Clearly, then, if my interpretion is correct, schedule_timeout() will
> always return to a state in msleep_interruptible() which causes the loop
> to only iterate the one time. Does this make sense? Is my interpretation
> of schedule_timeout()s functioning somehow flawed? If not, we probably
> can go ahead and change the msleep_interruptible() code, yes?
Ok, since nobody seems to have objected to my ideas as of yet, I went
ahead and implemented them, with much help from Domen and others. I
believe there are two options for these changes. One involves using
macros instead, the other involves reworking the functions.
I have attached here the version with macros (1/2). I am sure there will be
much to correct, so please do so.
Also, I will hopefully submit soon a change to a driver that uses this
new msleep*() interface, so as to show how it should work.
-Nish
Description: Removes definitions of msleep() and msleep_interruptible()
from kernel/timer.c in preparation for their re-creation as macros in
include/linux/delay.h. There are significant issues with the current
implementation of msleep_interruptible(). Primarily, the comments given
do not reflect the actual results. The function, in fact, ignores
wait-queue events and only will respond to signals. The new versions of
these functions will correct and clarify this.
--- 2.6.10-rc2-vanilla/kernel/timer.c 2004-11-19 16:12:28.000000000 -0800
+++ 2.6.10-rc2/kernel/timer.c 2004-11-19 16:47:47.000000000 -0800
@@ -1609,36 +1609,3 @@ unregister_time_interpolator(struct time
spin_unlock(&time_interpolator_lock);
}
#endif /* CONFIG_TIME_INTERPOLATION */
-
-/**
- * msleep - sleep safely even with waitqueue interruptions
- * @msecs: Time in milliseconds to sleep for
- */
-void msleep(unsigned int msecs)
-{
- unsigned long timeout = msecs_to_jiffies(msecs) + 1;
-
- while (timeout) {
- set_current_state(TASK_UNINTERRUPTIBLE);
- timeout = schedule_timeout(timeout);
- }
-}
-
-EXPORT_SYMBOL(msleep);
-
-/**
- * msleep_interruptible - sleep waiting for waitqueue interruptions
- * @msecs: Time in milliseconds to sleep for
- */
-unsigned long msleep_interruptible(unsigned int msecs)
-{
- unsigned long timeout = msecs_to_jiffies(msecs) + 1;
-
- while (timeout && !signal_pending(current)) {
- set_current_state(TASK_INTERRUPTIBLE);
- timeout = schedule_timeout(timeout);
- }
- return jiffies_to_msecs(timeout);
-}
-
-EXPORT_SYMBOL(msleep_interruptible);
next prev parent reply other threads:[~2004-11-20 0:53 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2004-11-17 2:49 schedule_timeout() issues / questions Nishanth Aravamudan
2004-11-20 0:48 ` Nishanth Aravamudan [this message]
2004-11-20 0:56 ` [PATCH 2/2] include/delay.h: replace msleep() and msleep_interruptible() with macros Nishanth Aravamudan
2004-11-20 1:23 ` Nishanth Aravamudan
2004-11-20 1:25 ` Nishanth Aravamudan
[not found] ` <200411201037.22237.oliver@neukum.org>
2004-11-22 18:01 ` Nishanth Aravamudan
[not found] ` <200411221934.53425.oliver@neukum.org>
2004-11-22 19:50 ` [PATCH ] kernel/timer: correct msleep_interruptible() comment Nishanth Aravamudan
2004-11-20 0:56 ` [PATCH 1/2] kernel/timer.c: remove msleep() and msleep_interruptible() Nishanth Aravamudan
2004-11-20 1:17 ` [PATCH 1/2] kernel/timer: remove msleep{,_interruptible}(); add __msleep_{sig,wq}() Nishanth Aravamudan
2004-11-20 1:21 ` [PATCH 2/2] include/delay: replace msleep{,_interruptible}() with macros Nishanth Aravamudan
2004-11-20 1:26 ` Nishanth Aravamudan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20041120004840.GA7466@us.ibm.com \
--to=nacc@us.ibm.com \
--cc=domen@coderock.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).