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?
This is closely tied to a separate issue. I have often seen code check
two separate conditions in response to calling schedule_timeout():
whether signals_pending() is true and whether the return value is 0 (in
separate if statements). Depending on the correctness of my interpretation
again, these are sequentially dependent. Meaning the following code should
be a correct and equivalent way to use schedule_timeout():
set_current_state(TASK_INTERRUPTIBLE);
timeout = schedule_timeout(timeout);
if (signals_pending(current))
/* respond appropriately to signals */
else
/* this must mean that timeout == 0, respond accordingly */
I think this should result in smaller, more uniform code and perhaps
more well-structured looping (as most schedule_timeout()s occur in some
loop). Does this if/else make sense? Since it is contingent on the
previous interpretation, maybe you've already thrown it out... But if
not, this may result in a good number of standardizing patches (which I
think is generally a good thing). This also would make it clear to me
how exactly the kernel interprets TASK_INTERRUPTIBLE /
TASK_UNINTERRUPTIBLE. <rant> Obviously, one's intuition that
TASK_UNINTERRUPTIBLE means don't wake up until the time as gone by,
at least, is wrong, or we wouldn't need the similar while-loop in
msleep()...
/**
* 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);
while (timeout) {
set_current_state(TASK_UNINTERRUPTIBLE);
timeout = schedule_timeout(timeout);
}
}
</rant>
2) schedule_timeout(1)
A second issue arises when considering the prevalence in the kernel of
calling
set_current_state(TASK_{,UN}INTERRUPTIBLE);
schedule_timeout(1);
The 1 is rather arbitrary (but most common); any small enough number
will do. I think these code segments are now "unintentional" (in the
majority, at least; in certain cases, they may certainly be intended).
The reason I think this, is that when HZ==100 this is a pretty long
delay in human-time (~10ms, assuming no signals...). So maybe code
authors actually intended for that length of a delay, but since msleep()
and family did not exist, just went with schedule_timeout(). Of course,
since 1 jiffy of delay was equivalent to 10ms, it makes the dependency
on HZ very hard to see. This is just a theory, though, and I could be
completely wrong.
The main point, though, is that this code now introduces excessive
overhead. Upon examining the code of schedule_timeout(), one sees the
gist of the function to be:
a) Set up a timer to go off after a certain number of jiffies
b) Call schedule()
c) Upon returning, delete the timer and return the number of
jiffies left (if positive) or 0
Now, with architectures where HZ==1000 or more, I think the overhead of
setting up the timer and then calling schedule() may result in the timer
going off in the middle of the schedule() call. This, of course, is not
great, because it could mean that another task will start up (for 1
jiffy!), thrash the cache perhaps, and then be preempted by my task
again. It would be better, I think to call schedule() directly in these
cases where the intention is: give up the CPU. It results in similar
behavior (excepting that I won't necessarily be able to run in a jiffy
or close to that time), but unless I misunderstand things, that is the
same situation as with the timer in schedule_timeout() (i.e., I might be
completely wrong, though, if somehow when the timer goes off, my task
gets put to the front of it's priority queue).
It just seems to me that we are putting in excessive code to just give
up the CPU (the intention of some of the calls, as I see it). I would like
to hear other opinions. If I am close to right, though, this will result
in a good number of (small) patches which will help clean up the kernel
a bit.
Please excuse my lack of brevity. There were a lot of ideas, and I
wanted to make myself pretty clear. Any comments would be greatly
appreciated.
Thanks,
Nish
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?
Forgot signed-off-by line...
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.
Signed-off-by: Nishanth Aravamudan <[email protected]>
--- 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);
On Fri, Nov 19, 2004 at 04:48:41PM -0800, Nishanth Aravamudan wrote:
> 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.
Here is patch 2/2, which modifies include/linux/delay.h to contain the
appropriate macros. Much consideration was given the various names and
calling syntax, but corrections/requests/changes are welcome.
-Nish
Description: Remove prototypes of msleep() and msleep_interruptible() to
prepare for the macro versions of these functions. Add macros for 4
types of sleeps:
1) Unconditional sleep (msleep)
2) Sleep until signalled (msleep_interruptible)
3) Sleep until wait-queue event (msleep_wq)
4) Sleep until either signalled or wait-queue event
(msleep_wq_interruptible)
These 4 cases are all distinct and handled separately by passing
appropriate flags to __msleep_sig and __msleep_wq, which should *not*
ever be called directly by kernel code.
Signed-off-by: Nishanth Aravamudan <[email protected]>
--- 2.6.10-rc2-vanilla/include/linux/delay.h 2004-11-19 16:11:52.000000000 -0800
+++ 2.6.10-rc2/include/linux/delay.h 2004-11-19 16:52:40.000000000 -0800
@@ -38,8 +38,56 @@ extern unsigned long loops_per_jiffy;
#define ndelay(x) udelay(((x)+999)/1000)
#endif
-void msleep(unsigned int msecs);
-unsigned long msleep_interruptible(unsigned int msecs);
+/*
+ * Sleep in state while condition is true
+ */
+#define __msleep_sig(unsigned int msecs, long state, int condition) \
+ do { \
+ unsigned long timeout = msecs_to_jiffies(msecs) + 1; \
+ \
+ while (timeout && condition) { \
+ set_current_state(state); \
+ timeout = schedule_timeout(timeout); \
+ } \
+ \
+ return jiffies_to_msecs(timeout); \
+ } while(0)
+
+/*
+ * Sleep in state until schedule_timeout() returns
+ */
+#define __msleep_wq(unsigned int msecs, long state) \
+ do { \
+ unsigned long timeout = msecs_to_jiffies(msecs) + 1; \
+ \
+ set_current_state(condition); \
+ timeout = schedule_timeout(timeout); \
+ \
+ return jiffies_to_msecs(timeout); \
+ } while(0)
+
+/*
+ * Sleep until at least msecs ms have elapsed
+ */
+#define msleep(msecs) (void)__msleep_sig(msecs, TASK_UNINTERRUPTIBLE, 1)
+
+/*
+ * Sleep until at least msecs ms have elapsed or a signal is received
+ */
+#define msleep_interruptible(msecs) \
+ __msleep_sig(msecs, TASK_INTERRUPTIBLE, !signals_pending(current))
+
+/*
+ * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
+ */
+#define msleep_wq(msecs) __msleep_wq_state(msecs, TASK_UNINTERRUPTIBLE)
+
+/*
+ * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
+ * or a signal is received
+ */
+#define msleep_wq_interruptible(msecs) \
+ __msleep_wq_state(msecs, TASK_INTERRUPTIBLE);
static inline void ssleep(unsigned int seconds)
{
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);
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?
Here is the function variant of the previous patches.
Description: Remove msleep() and msleep_interruptible(). Add function
definitions for __msleep_sig() and __msleep_wq(). These functions are
the "hidden" backends to the new implementations of the msleep() class
of functions. They should avoid many of the problems with the existing
system by being clearer on what they are sleeping for.
Signed-off-by: Nishanth Aravamudan <[email protected]>
--- 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 17:13:13.000000000 -0800
@@ -1610,35 +1610,30 @@ unregister_time_interpolator(struct time
}
#endif /* CONFIG_TIME_INTERPOLATION */
-/**
- * msleep - sleep safely even with waitqueue interruptions
- * @msecs: Time in milliseconds to sleep for
+/*
+ * Sleep in state and, if check sigs, until signal received
*/
-void msleep(unsigned int msecs)
+unsigned int __msleep_sig(unsigned int msecs, long state, int check_sigs)
{
unsigned long timeout = msecs_to_jiffies(msecs) + 1;
-
- while (timeout) {
- set_current_state(TASK_UNINTERRUPTIBLE);
+
+ while (timeout && (check_sigs ? !signals_pending(current) : 1)) {
+ set_current_state(state);
timeout = schedule_timeout(timeout);
}
-}
-EXPORT_SYMBOL(msleep);
+ return jiffies_to_msecs(timeout);
+}
-/**
- * msleep_interruptible - sleep waiting for waitqueue interruptions
- * @msecs: Time in milliseconds to sleep for
+/*
+ * Sleep in state until schedule_timeout() returns
*/
-unsigned long msleep_interruptible(unsigned int msecs)
+unsigned int __msleep_wq(unsigned int msecs, long state)
{
unsigned long timeout = msecs_to_jiffies(msecs) + 1;
-
- while (timeout && !signal_pending(current)) {
- set_current_state(TASK_INTERRUPTIBLE);
- timeout = schedule_timeout(timeout);
- }
+
+ set_current_state(state);
+ timeout = schedule_timeout(timeout);
+
return jiffies_to_msecs(timeout);
}
-
-EXPORT_SYMBOL(msleep_interruptible);
On Fri, Nov 19, 2004 at 05:17:51PM -0800, Nishanth Aravamudan wrote:
> 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?
>
> Here is the function variant of the previous patches.
Here is the corresponding change to delay.h. The major difference from
the macro version is that a flag is now passed to __msleep_sig() to
determine whether an extra condition is necessary in the while() loop.
-Nish
Description: Add macros for various sleep functions, one for each case
of:
Description: Remove prototypes of msleep() and msleep_interruptible() to
prepare for the macro versions of these functions. Add prototypes for
__msleep_wq() and __msleep_sig(). Add macros for 4 types of sleeps:
1) Unconditional sleep (msleep)
2) Sleep until signalled (msleep_interruptible)
3) Sleep until wait-queue event (msleep_wq)
4) Sleep until either signalled or wait-queue event
(msleep_wq_interruptible)
These 4 cases are all distinct and handled separately by passing
appropriate flags to __msleep_sig and __msleep_wq, which should *not*
ever be called directly by kernel code.
Signed-off-by: Nishanth Aravamudan <[email protected]>
--- 2.6.10-rc2-vanilla/include/linux/delay.h 2004-11-19 16:11:52.000000000 -0800
+++ 2.6.10-rc2/include/linux/delay.h 2004-11-19 17:12:47.000000000 -0800
@@ -38,8 +38,31 @@ extern unsigned long loops_per_jiffy;
#define ndelay(x) udelay(((x)+999)/1000)
#endif
-void msleep(unsigned int msecs);
-unsigned long msleep_interruptible(unsigned int msecs);
+unsigned int __msleep_sig(unsigned int msecs, long state, int check_sigs);
+unsigned int __msleep_wq(unsigned int msecs, long state);
+
+/*
+ * Sleep until at least msecs ms have elapsed
+ */
+#define msleep(msecs) (void)__msleep_sig(msecs, TASK_UNINTERRUPTIBLE, 0)
+
+/*
+ * Sleep until at least msecs ms have elapsed or a signal is received
+ */
+#define msleep_interruptible(msecs) \
+ __msleep_sig(msecs, TASK_INTERRUPTIBLE, 1))
+
+/*
+ * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
+ */
+#define msleep_wq(msecs) __msleep_wq_state(msecs, TASK_UNINTERRUPTIBLE)
+
+/*
+ * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
+ * or a signal is received
+ */
+#define msleep_wq_interruptible(msecs) \
+ __msleep_wq_state(msecs, TASK_INTERRUPTIBLE);
static inline void ssleep(unsigned int seconds)
{
On Fri, Nov 19, 2004 at 04:56:01PM -0800, Nishanth Aravamudan wrote:
> On Fri, Nov 19, 2004 at 04:48:41PM -0800, Nishanth Aravamudan wrote:
> > 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.
>
> Here is patch 2/2, which modifies include/linux/delay.h to contain the
> appropriate macros. Much consideration was given the various names and
> calling syntax, but corrections/requests/changes are welcome.
>
> -Nish
>
> Description: Remove prototypes of msleep() and msleep_interruptible() to
> prepare for the macro versions of these functions. Add macros for 4
> types of sleeps:
> 1) Unconditional sleep (msleep)
> 2) Sleep until signalled (msleep_interruptible)
> 3) Sleep until wait-queue event (msleep_wq)
> 4) Sleep until either signalled or wait-queue event
> (msleep_wq_interruptible)
> These 4 cases are all distinct and handled separately by passing
> appropriate flags to __msleep_sig and __msleep_wq, which should *not*
> ever be called directly by kernel code.
>
> Signed-off-by: Nishanth Aravamudan <[email protected]>
>
> --- 2.6.10-rc2-vanilla/include/linux/delay.h 2004-11-19 16:11:52.000000000 -0800
> +++ 2.6.10-rc2/include/linux/delay.h 2004-11-19 16:52:40.000000000 -0800
> @@ -38,8 +38,56 @@ extern unsigned long loops_per_jiffy;
> #define ndelay(x) udelay(((x)+999)/1000)
> #endif
>
> -void msleep(unsigned int msecs);
> -unsigned long msleep_interruptible(unsigned int msecs);
> +/*
> + * Sleep in state while condition is true
> + */
> +#define __msleep_sig(unsigned int msecs, long state, int condition) \
> + do { \
> + unsigned long timeout = msecs_to_jiffies(msecs) + 1; \
> + \
> + while (timeout && condition) { \
> + set_current_state(state); \
> + timeout = schedule_timeout(timeout); \
> + } \
> + \
> + return jiffies_to_msecs(timeout); \
> + } while(0)
> +
> +/*
> + * Sleep in state until schedule_timeout() returns
> + */
> +#define __msleep_wq(unsigned int msecs, long state) \
> + do { \
> + unsigned long timeout = msecs_to_jiffies(msecs) + 1; \
> + \
> + set_current_state(condition); \
Obvious typo, fixed in patch below...
Description: Remove prototypes of msleep() and msleep_interruptible() to
prepare for the macro versions of these functions. Add macros for 4
types of sleeps:
1) Unconditional sleep (msleep)
2) Sleep until signalled (msleep_interruptible)
3) Sleep until wait-queue event (msleep_wq)
4) Sleep until either signalled or wait-queue event
(msleep_wq_interruptible)
These 4 cases are all distinct and handled separately by passing
appropriate flags to __msleep_sig and __msleep_wq, which should *not*
ever be called directly by kernel code.
Signed-off-by: Nishanth Aravamudan <[email protected]>
--- 2.6.10-rc2-vanilla/include/linux/delay.h 2004-11-19 16:11:52.000000000 -0800
+++ 2.6.10-rc2/include/linux/delay.h 2004-11-19 16:52:40.000000000 -0800
@@ -38,8 +38,56 @@ extern unsigned long loops_per_jiffy;
#define ndelay(x) udelay(((x)+999)/1000)
#endif
-void msleep(unsigned int msecs);
-unsigned long msleep_interruptible(unsigned int msecs);
+/*
+ * Sleep in state while condition is true
+ */
+#define __msleep_sig(unsigned int msecs, long state, int condition) \
+ do { \
+ unsigned long timeout = msecs_to_jiffies(msecs) + 1; \
+ \
+ while (timeout && condition) { \
+ set_current_state(state); \
+ timeout = schedule_timeout(timeout); \
+ } \
+ \
+ return jiffies_to_msecs(timeout); \
+ } while(0)
+
+/*
+ * Sleep in state until schedule_timeout() returns
+ */
+#define __msleep_wq(unsigned int msecs, long state) \
+ do { \
+ unsigned long timeout = msecs_to_jiffies(msecs) + 1; \
+ \
+ set_current_state(state); \
+ timeout = schedule_timeout(timeout); \
+ \
+ return jiffies_to_msecs(timeout); \
+ } while(0)
+
+/*
+ * Sleep until at least msecs ms have elapsed
+ */
+#define msleep(msecs) (void)__msleep_sig(msecs, TASK_UNINTERRUPTIBLE, 1)
+
+/*
+ * Sleep until at least msecs ms have elapsed or a signal is received
+ */
+#define msleep_interruptible(msecs) \
+ __msleep_sig(msecs, TASK_INTERRUPTIBLE, !signals_pending(current))
+
+/*
+ * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
+ */
+#define msleep_wq(msecs) __msleep_wq_state(msecs, TASK_UNINTERRUPTIBLE)
+
+/*
+ * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
+ * or a signal is received
+ */
+#define msleep_wq_interruptible(msecs) \
+ __msleep_wq_state(msecs, TASK_INTERRUPTIBLE);
static inline void ssleep(unsigned int seconds)
{
On Fri, Nov 19, 2004 at 05:23:32PM -0800, Nishanth Aravamudan wrote:
> On Fri, Nov 19, 2004 at 04:56:01PM -0800, Nishanth Aravamudan wrote:
> > On Fri, Nov 19, 2004 at 04:48:41PM -0800, Nishanth Aravamudan wrote:
> > > 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.
> >
> > Here is patch 2/2, which modifies include/linux/delay.h to contain the
> > appropriate macros. Much consideration was given the various names and
> > calling syntax, but corrections/requests/changes are welcome.
> >
> > -Nish
> >
> > Description: Remove prototypes of msleep() and msleep_interruptible() to
> > prepare for the macro versions of these functions. Add macros for 4
> > types of sleeps:
> > 1) Unconditional sleep (msleep)
> > 2) Sleep until signalled (msleep_interruptible)
> > 3) Sleep until wait-queue event (msleep_wq)
> > 4) Sleep until either signalled or wait-queue event
> > (msleep_wq_interruptible)
> > These 4 cases are all distinct and handled separately by passing
> > appropriate flags to __msleep_sig and __msleep_wq, which should *not*
> > ever be called directly by kernel code.
> >
> > Signed-off-by: Nishanth Aravamudan <[email protected]>
> >
> > --- 2.6.10-rc2-vanilla/include/linux/delay.h 2004-11-19 16:11:52.000000000 -0800
> > +++ 2.6.10-rc2/include/linux/delay.h 2004-11-19 16:52:40.000000000 -0800
> > @@ -38,8 +38,56 @@ extern unsigned long loops_per_jiffy;
> > #define ndelay(x) udelay(((x)+999)/1000)
> > #endif
> >
> > -void msleep(unsigned int msecs);
> > -unsigned long msleep_interruptible(unsigned int msecs);
> > +/*
> > + * Sleep in state while condition is true
> > + */
> > +#define __msleep_sig(unsigned int msecs, long state, int condition) \
> > + do { \
> > + unsigned long timeout = msecs_to_jiffies(msecs) + 1; \
> > + \
> > + while (timeout && condition) { \
> > + set_current_state(state); \
> > + timeout = schedule_timeout(timeout); \
> > + } \
> > + \
> > + return jiffies_to_msecs(timeout); \
> > + } while(0)
> > +
> > +/*
> > + * Sleep in state until schedule_timeout() returns
> > + */
> > +#define __msleep_wq(unsigned int msecs, long state) \
> > + do { \
> > + unsigned long timeout = msecs_to_jiffies(msecs) + 1; \
> > + \
> > + set_current_state(condition); \
>
> Obvious typo, fixed in patch below...
>
>
> Description: Remove prototypes of msleep() and msleep_interruptible() to
> prepare for the macro versions of these functions. Add macros for 4
> types of sleeps:
> 1) Unconditional sleep (msleep)
> 2) Sleep until signalled (msleep_interruptible)
> 3) Sleep until wait-queue event (msleep_wq)
> 4) Sleep until either signalled or wait-queue event
> (msleep_wq_interruptible)
> These 4 cases are all distinct and handled separately by passing
> appropriate flags to __msleep_sig and __msleep_wq, which should *not*
> ever be called directly by kernel code.
>
> Signed-off-by: Nishanth Aravamudan <[email protected]>
>
>
> --- 2.6.10-rc2-vanilla/include/linux/delay.h 2004-11-19 16:11:52.000000000 -0800
> +++ 2.6.10-rc2/include/linux/delay.h 2004-11-19 16:52:40.000000000 -0800
> @@ -38,8 +38,56 @@ extern unsigned long loops_per_jiffy;
> #define ndelay(x) udelay(((x)+999)/1000)
> #endif
>
> -void msleep(unsigned int msecs);
> -unsigned long msleep_interruptible(unsigned int msecs);
> +/*
> + * Sleep in state while condition is true
> + */
> +#define __msleep_sig(unsigned int msecs, long state, int condition) \
> + do { \
> + unsigned long timeout = msecs_to_jiffies(msecs) + 1; \
> + \
> + while (timeout && condition) { \
> + set_current_state(state); \
> + timeout = schedule_timeout(timeout); \
> + } \
> + \
> + return jiffies_to_msecs(timeout); \
> + } while(0)
> +
> +/*
> + * Sleep in state until schedule_timeout() returns
> + */
> +#define __msleep_wq(unsigned int msecs, long state) \
> + do { \
> + unsigned long timeout = msecs_to_jiffies(msecs) + 1; \
> + \
> + set_current_state(state); \
> + timeout = schedule_timeout(timeout); \
> + \
> + return jiffies_to_msecs(timeout); \
> + } while(0)
> +
> +/*
> + * Sleep until at least msecs ms have elapsed
> + */
> +#define msleep(msecs) (void)__msleep_sig(msecs, TASK_UNINTERRUPTIBLE, 1)
> +
> +/*
> + * Sleep until at least msecs ms have elapsed or a signal is received
> + */
> +#define msleep_interruptible(msecs) \
> + __msleep_sig(msecs, TASK_INTERRUPTIBLE, !signals_pending(current))
> +
> +/*
> + * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
> + */
> +#define msleep_wq(msecs) __msleep_wq_state(msecs, TASK_UNINTERRUPTIBLE)
> +
> +/*
> + * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
> + * or a signal is received
> + */
> +#define msleep_wq_interruptible(msecs) \
> + __msleep_wq_state(msecs, TASK_INTERRUPTIBLE);
Another stupid typo.
Description: Remove prototypes of msleep() and msleep_interruptible() to
prepare for the macro versions of these functions. Add macros for 4
types of sleeps:
1) Unconditional sleep (msleep)
2) Sleep until signalled (msleep_interruptible)
3) Sleep until wait-queue event (msleep_wq)
4) Sleep until either signalled or wait-queue event
(msleep_wq_interruptible)
These 4 cases are all distinct and handled separately by passing
appropriate flags to __msleep_sig and __msleep_wq, which should *not*
ever be called directly by kernel code.
Signed-off-by: Nishanth Aravamudan <[email protected]>
--- 2.6.10-rc2-vanilla/include/linux/delay.h 2004-11-19 16:11:52.000000000 -0800
+++ 2.6.10-rc2/include/linux/delay.h 2004-11-19 16:52:40.000000000 -0800
@@ -38,8 +38,56 @@ extern unsigned long loops_per_jiffy;
#define ndelay(x) udelay(((x)+999)/1000)
#endif
-void msleep(unsigned int msecs);
-unsigned long msleep_interruptible(unsigned int msecs);
+/*
+ * Sleep in state while condition is true
+ */
+#define __msleep_sig(unsigned int msecs, long state, int condition) \
+ do { \
+ unsigned long timeout = msecs_to_jiffies(msecs) + 1; \
+ \
+ while (timeout && condition) { \
+ set_current_state(state); \
+ timeout = schedule_timeout(timeout); \
+ } \
+ \
+ return jiffies_to_msecs(timeout); \
+ } while(0)
+
+/*
+ * Sleep in state until schedule_timeout() returns
+ */
+#define __msleep_wq(unsigned int msecs, long state) \
+ do { \
+ unsigned long timeout = msecs_to_jiffies(msecs) + 1; \
+ \
+ set_current_state(condition); \
+ timeout = schedule_timeout(timeout); \
+ \
+ return jiffies_to_msecs(timeout); \
+ } while(0)
+
+/*
+ * Sleep until at least msecs ms have elapsed
+ */
+#define msleep(msecs) (void)__msleep_sig(msecs, TASK_UNINTERRUPTIBLE, 1)
+
+/*
+ * Sleep until at least msecs ms have elapsed or a signal is received
+ */
+#define msleep_interruptible(msecs) \
+ __msleep_sig(msecs, TASK_INTERRUPTIBLE, !signals_pending(current))
+
+/*
+ * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
+ */
+#define msleep_wq(msecs) __msleep_wq_state(msecs, TASK_UNINTERRUPTIBLE)
+
+/*
+ * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
+ * or a signal is received
+ */
+#define msleep_wq_interruptible(msecs) \
+ __msleep_wq_state(msecs, TASK_INTERRUPTIBLE)
static inline void ssleep(unsigned int seconds)
{
On Fri, Nov 19, 2004 at 05:21:53PM -0800, Nishanth Aravamudan wrote:
> On Fri, Nov 19, 2004 at 05:17:51PM -0800, Nishanth Aravamudan wrote:
> > 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?
> >
> > Here is the function variant of the previous patches.
>
> Here is the corresponding change to delay.h. The major difference from
> the macro version is that a flag is now passed to __msleep_sig() to
> determine whether an extra condition is necessary in the while() loop.
>
> -Nish
>
> Description: Add macros for various sleep functions, one for each case
> of:
>
> Description: Remove prototypes of msleep() and msleep_interruptible() to
> prepare for the macro versions of these functions. Add prototypes for
> __msleep_wq() and __msleep_sig(). Add macros for 4 types of sleeps:
> 1) Unconditional sleep (msleep)
> 2) Sleep until signalled (msleep_interruptible)
> 3) Sleep until wait-queue event (msleep_wq)
> 4) Sleep until either signalled or wait-queue event
> (msleep_wq_interruptible)
> These 4 cases are all distinct and handled separately by passing
> appropriate flags to __msleep_sig and __msleep_wq, which should *not*
> ever be called directly by kernel code.
>
>
> Signed-off-by: Nishanth Aravamudan <[email protected]>
>
>
> --- 2.6.10-rc2-vanilla/include/linux/delay.h 2004-11-19 16:11:52.000000000 -0800
> +++ 2.6.10-rc2/include/linux/delay.h 2004-11-19 17:12:47.000000000 -0800
> @@ -38,8 +38,31 @@ extern unsigned long loops_per_jiffy;
> #define ndelay(x) udelay(((x)+999)/1000)
> #endif
>
> -void msleep(unsigned int msecs);
> -unsigned long msleep_interruptible(unsigned int msecs);
> +unsigned int __msleep_sig(unsigned int msecs, long state, int check_sigs);
> +unsigned int __msleep_wq(unsigned int msecs, long state);
> +
> +/*
> + * Sleep until at least msecs ms have elapsed
> + */
> +#define msleep(msecs) (void)__msleep_sig(msecs, TASK_UNINTERRUPTIBLE, 0)
> +
> +/*
> + * Sleep until at least msecs ms have elapsed or a signal is received
> + */
> +#define msleep_interruptible(msecs) \
> + __msleep_sig(msecs, TASK_INTERRUPTIBLE, 1))
> +
> +/*
> + * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
> + */
> +#define msleep_wq(msecs) __msleep_wq_state(msecs, TASK_UNINTERRUPTIBLE)
> +
> +/*
> + * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
> + * or a signal is received
> + */
> +#define msleep_wq_interruptible(msecs) \
> + __msleep_wq_state(msecs, TASK_INTERRUPTIBLE);
A stupid typo...
Description: Remove prototypes of msleep() and msleep_interruptible() to
prepare for the macro versions of these functions. Add prototypes for
__msleep_wq() and __msleep_sig(). Add macros for 4 types of sleeps:
1) Unconditional sleep (msleep)
2) Sleep until signalled (msleep_interruptible)
3) Sleep until wait-queue event (msleep_wq)
4) Sleep until either signalled or wait-queue event
(msleep_wq_interruptible)
These 4 cases are all distinct and handled separately by passing
appropriate flags to __msleep_sig and __msleep_wq, which should *not*
ever be called directly by kernel code.
Signed-off-by: Nishanth Aravamudan <[email protected]>
--- 2.6.10-rc2-vanilla/include/linux/delay.h 2004-11-19 16:11:52.000000000 -0800
+++ 2.6.10-rc2/include/linux/delay.h 2004-11-19 17:12:47.000000000 -0800
@@ -38,8 +38,31 @@ extern unsigned long loops_per_jiffy;
#define ndelay(x) udelay(((x)+999)/1000)
#endif
-void msleep(unsigned int msecs);
-unsigned long msleep_interruptible(unsigned int msecs);
+unsigned int __msleep_sig(unsigned int msecs, long state, int check_sigs);
+unsigned int __msleep_wq(unsigned int msecs, long state);
+
+/*
+ * Sleep until at least msecs ms have elapsed
+ */
+#define msleep(msecs) (void)__msleep_sig(msecs, TASK_UNINTERRUPTIBLE, 0)
+
+/*
+ * Sleep until at least msecs ms have elapsed or a signal is received
+ */
+#define msleep_interruptible(msecs) \
+ __msleep_sig(msecs, TASK_INTERRUPTIBLE, 1))
+
+/*
+ * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
+ */
+#define msleep_wq(msecs) __msleep_wq_state(msecs, TASK_UNINTERRUPTIBLE)
+
+/*
+ * Sleep until at least msecs ms have elapsed or a wait-queue event occurs
+ * or a signal is received
+ */
+#define msleep_wq_interruptible(msecs) \
+ __msleep_wq_state(msecs, TASK_INTERRUPTIBLE)
static inline void ssleep(unsigned int seconds)
{
On Sat, Nov 20, 2004 at 10:37:21AM +0100, Oliver Neukum wrote:
> Am Samstag, 20. November 2004 02:23 schrieb Nishanth Aravamudan:
> > Description: Remove prototypes of msleep() and msleep_interruptible() to
> > prepare for the macro versions of these functions. Add macros for 4
> > types of sleeps:
>
> What is the purpose of having macros for delay?
> They are on a slow path by definition. You want the smallest possible
> function call here, nothing fancy.
Just so I'm clear on what you are asking... Do you mean why am I using
macros or why the macros are needed at all?
To the former, I am more than happy to change them to functions, and, in
fact, I believe I have to for modules (EXPORT_SYMBOL_GPL?) to be able to
call the sleep functions.
To the latter, having these 4 functions/macros makes it clear for what
reason you are sleeping. It leads to *correct* functionality of the
code, which we do not currently have.
-Nish