Skip to contentSkip to conversation

ieee/sbp2: replace schedule_timeout() with

PATCH In review Nishanth Aravamudan 4 participants 5 messages Jul 22, 2004 – Jul 23, 2004 Kernel janitors

lore
Share & tools
lore

Nishanth Aravamudan

author
#0 lore raw

PATCH 1/1 ieee/sbp2: replace schedule_timeout() with

I would appreciate any comments from the janitors list. This is one (of many) cases where I made a decision about replacing

set_current_state(TASK_INTERRUPTIBLE);
schedule_timeout(some_time);

with

msleep(jiffies_to_msecs(some_time));

msleep() is not exactly the same as the previous code, but I only did this replacement where I thought long delays were *desired*. If this is not the case here, then just disregard this patch.

Thanks,
Nish

Applys-to: 2.6.7

Description: Use msleep() instead of schedule_timeout() to guarantee desired timeout.

Signed-off-by Nishanth Aravamudan <[email protected]>
drivers/ieee1394/sbp2.c +1 -2
@@ -903,8 +903,7 @@ alloc_fail:
 	 * connected to the sbp2 device being removed. That host would
 	 * have a certain amount of time to relogin before the sbp2 device
 	 * allows someone else to login instead. One second makes sense. */
-	set_current_state(TASK_INTERRUPTIBLE);
-	schedule_timeout(HZ);
+	msleep(1000);
 
 	/*
 	 * Login to the sbp-2 device

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#1 lore raw

Re: [Kernel-janitors] [PATCH] ieee/sbp2: replace schedule_timeout()

Quoted 6 lines · Nishanth Aravamudan

I would appreciate any comments from the janitors list. This is one (of many) cases where I made a decision about replacing

set_current_state(TASK_INTERRUPTIBLE);
schedule_timeout(some_time);

Quoted 7 lines

with

msleep(jiffies_to_msecs(some_time));

msleep() is not exactly the same as the previous code, but I only did this replacement where I thought long delays were *desired*. If this is not the case here, then just disregard this patch.

Doesn't msleep() block? If so, that's not what we want at all. We want other things to happen during that time (like maybe catch a bus reset).

Signature · 4 lines
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#2 lore raw

Re: [Kernel-janitors] [PATCH] ieee/sbp2: replace schedule_timeout()

In reply to Nishanth Aravamudan
Quoted 2 lines · Ben Collins

Doesn't msleep() block? If so, that's not what we want at all. We want other things to happen during that time (like maybe catch a bus reset).

No, msleep sleeps (as its name suggests). The code that implements it is:

void msleep(unsigned int msecs)

{
        unsigned long timeout = msecs_to_jiffies(msecs);

        while (timeout) {
                set_current_state(TASK_UNINTERRUPTIBLE);
                timeout = schedule_timeout(timeout);
        }
}
Signature · 6 lines
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#3 lore raw

Re: [Kernel-janitors] [PATCH] ieee/sbp2: replace schedule_timeout()

In reply to Nishanth Aravamudan
Quoted 5 lines · Matthew Wilcox
Quoted 2 lines · Ben Collins

Doesn't msleep() block? If so, that's not what we want at all. We want other things to happen during that time (like maybe catch a bus reset).

No, msleep sleeps (as its name suggests). The code that implements it is:

In that case, I've got a few places that could use msleep().

Thanks.

Signature · 4 lines
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#4 lore raw

Re: Re: [Kernel-janitors] [PATCH] ieee/sbp2: replace

In reply to Nishanth Aravamudan
Quoted 8 lines · Ben Collins
Quoted 5 lines · Matthew Wilcox
Quoted 2 lines · Ben Collins

Doesn't msleep() block? If so, that's not what we want at all. We want other things to happen during that time (like maybe catch a bus reset).

No, msleep sleeps (as its name suggests). The code that implements it is:

In that case, I've got a few places that could use msleep().

The one thing is, it depends on what you mean by "like maybe catch a bus reset" (I think), because msleep will sleep through all signals, i.e. it provides a guaranteed minimum delay. In the case of my patch, I didn't see you checking for any signals afterwards (or any comments indicating you were expecting some) so I figured it would be safe to use msleep(). Also, I think it's clearer what's going on if the time units for delays are msecs.

-Nish

PS. Would these other places be in drivers as well?

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors