[Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout() with

[Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout() with

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

Hi!

> I would appreciate any comments from the janitors list.
> 
> 
> Applys-to: 2.6.7
> 
> Description: Uses msleep() instead of schedule_timeout() to guarantee
> the task delays at least the desired time amount.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>

I'm not sure it makes sense.... so it will just spin once more? Exact
timeout does not seem too critical here.
								Pavel

> --- linux-vanilla/drivers/block/nbd.c	2004-06-16 05:19:01.000000000 +0000
> +++ linux-dev/drivers/block/nbd.c	2004-07-02 17:24:31.000000000 +0000
> @@ -139,8 +139,7 @@ static void nbd_end_request(struct reque
>  		spin_unlock(&lo->queue_lock);
>  		printk(KERN_DEBUG "%s: request %p still in use (%d), waiting\n",
>  		    lo->disk->disk_name, req, req->ref_count);
> -		set_current_state(TASK_UNINTERRUPTIBLE);
> -		schedule_timeout(HZ); /* wait a second */
> +		msleep(1000);
>  		spin_lock(&lo->queue_lock);
>  	}
>  	spin_unlock(&lo->queue_lock);

-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout()

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 303 bytes --]

Hi!

> >I'm not sure it makes sense.... so it will just spin once more? Exact
> >timeout does not seem too critical here.
> 
> 	So why not use HZ instead of 1000?

Hm, using constant like 1000 probably is not good idea.

						Pavel
-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout()

[Felipe W Damasio]

[-- Attachment #1: Type: text/plain, Size: 283 bytes --]



Pavel Machek wrote:
> I'm not sure it makes sense.... so it will just spin once more? Exact
> timeout does not seem too critical here.

	So why not use HZ instead of 1000?

Felipe
-- 
It's most certainly GNU/Linux, not Linux. Read more at
http://www.gnu.org/gnu/why-gnu-linux.html

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout()

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 658 bytes --]

On Tue, Jul 20, 2004 at 10:10:31AM -0300, Felipe W Damasio wrote:
> 
> 
> Pavel Machek wrote:
> >I'm not sure it makes sense.... so it will just spin once more? Exact
> >timeout does not seem too critical here.
> 
> 	So why not use HZ instead of 1000?

Well, msleep() is in terms of milliseconds while schedule_timeout() is in terms
of jiffies. Thus, you would use HZ if you called schedule_timeout() [and
it would always sleep (at most) one second, because HZ counts the number
of jiffies in one second] and you would use 1000 if you called msleep()
[and it would always sleep (at least) one second, because there are 1000
milliseconds in 1 second].

-Nish

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout()

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

On Tue, Jul 20, 2004 at 03:08:34PM +0200, Pavel Machek wrote:
> Hi!
> 
> > >I'm not sure it makes sense.... so it will just spin once more? Exact
> > >timeout does not seem too critical here.
> > 
> > 	So why not use HZ instead of 1000?
> 
> Hm, using constant like 1000 probably is not good idea.

Well, I thought it would be better than adding a #define for the number
of milliseconds in one second. Also, any rounding conditions or what-not
are taken care of within msleep() by the call to msecs_to_jiffies().

-Nish

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout() with

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

On Tue, Jul 20, 2004 at 03:02:44PM +0200, Pavel Machek wrote:
> Hi!
> 
> > I would appreciate any comments from the janitors list.
> > 
> > 
> > Applys-to: 2.6.7
> > 
> > Description: Uses msleep() instead of schedule_timeout() to guarantee
> > the task delays at least the desired time amount.
> > 
> > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> 
> I'm not sure it makes sense.... so it will just spin once more? Exact
> timeout does not seem too critical here.

In this case, because the state was set to TASK_UNINTERRUPTIBLE, the
exact timeout was desired (also factoring in the comment). There are
cases, though, where TASK_UNINTERRUPTIBLE'd schedule_timeout()s will not
last the full duration. msleep(), in contrast, provides exactly this
guarantee by placing the schedule_timeout() in a tight while loop on the
time remaining in the timeout.

Does that make some more sense?

-Nish

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout()

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

Hi!

> > > >I'm not sure it makes sense.... so it will just spin once more? Exact
> > > >timeout does not seem too critical here.
> > > 
> > > 	So why not use HZ instead of 1000?
> > 
> > Hm, using constant like 1000 probably is not good idea.
> 
> Well, I thought it would be better than adding a #define for the number
> of milliseconds in one second. Also, any rounding conditions or what-not
> are taken care of within msleep() by the call to msecs_to_jiffies().

Sorry, I did not check back the sources. mdelay(1000) is okay.

But I believe replacing schedule-timeout() with mdelay is not good
idea as it is okay to sleep shorter time at this particular place.

							Pavel
-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

[Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout() with

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

Hi!

> > > I would appreciate any comments from the janitors list.
> > > 
> > > 
> > > Applys-to: 2.6.7
> > > 
> > > Description: Uses msleep() instead of schedule_timeout() to guarantee
> > > the task delays at least the desired time amount.
> > > 
> > > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> > 
> > I'm not sure it makes sense.... so it will just spin once more? Exact
> > timeout does not seem too critical here.
> 
> In this case, because the state was set to TASK_UNINTERRUPTIBLE, the
> exact timeout was desired (also factoring in the comment). There are
> cases, though, where TASK_UNINTERRUPTIBLE'd schedule_timeout()s will not
> last the full duration. msleep(), in contrast, provides exactly this
> guarantee by placing the schedule_timeout() in a tight while loop on the
> time remaining in the timeout.

Are you sure this particular place is not okay with sleeping less
time? Having "sleep a while" comments suggests it does not care about
exactly one second. UNINTERRUPTIBLE may be unrelated.

-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout()

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]

On Tue, Jul 20, 2004 at 06:24:48PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > >I'm not sure it makes sense.... so it will just spin once more? Exact
> > > > >timeout does not seem too critical here.
> > > > 
> > > > 	So why not use HZ instead of 1000?
> > > 
> > > Hm, using constant like 1000 probably is not good idea.
> > 
> > Well, I thought it would be better than adding a #define for the number
> > of milliseconds in one second. Also, any rounding conditions or what-not
> > are taken care of within msleep() by the call to msecs_to_jiffies().
> 
> Sorry, I did not check back the sources. mdelay(1000) is okay.
> 
> But I believe replacing schedule-timeout() with mdelay is not good
> idea as it is okay to sleep shorter time at this particular place.

First, just to make sure we are on the same page, the replacement is
with msleep(), not mdelay(). The difference between the two is that
msleep() uses schedule_timeout() to give up the CPU while mdelay() busy
waits.

Second, according to the description at
kernel/timer.c::schedule_timeout(), I am just making the desired outcome
guaranteed:

 * %TASK_UNINTERRUPTIBLE - at least @timeout jiffies are guaranteed to
 * pass before the routine returns. The routine will return 0

As I said before, msleep() *guarantees* this behavior.

Also, I am following one of the TODO tasks:

  Addendum From:  Greg KH

  -> most of the longer delays in the kernel can be converted to use
   msleep() as it will work properly. drivers should it  instead of 
    creating their own (incorrect) function. 
     (example: sony_sleep(void) in drivers/cdrom/sonycd535.c)

After discussing with Greg, longer delays were considered those
measurable in milliseconds. I think msleep() helps add a layer of
consistency across the board. It also is (IMO) easier to think of times
and delays in terms of seconds / milliseconds rather than jiffies (this
becomes very apparent in later patches, where all sorts of crazy
conversions are replaced with simple numbers).

-Nish
  

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout()

[Nishanth Aravamudan]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

On Tue, Jul 20, 2004 at 10:10:31AM -0300, Felipe W Damasio wrote:
> 
> 
> Pavel Machek wrote:
> >I'm not sure it makes sense.... so it will just spin once more? Exact
> >timeout does not seem too critical here.
> 
> 	So why not use HZ instead of 1000?

Because msleep() is in terms of milliseconds, while schedule_timeout()
is in terms of jiffies. Thus, to sleep for a second, one would
schedule_timeout(HZ); (setting the state appropriately beforehand) or
(to guarantee the delay) msleep(1000);

Thanks,
Nish

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout()

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 507 bytes --]

Hi!

> > But I believe replacing schedule-timeout() with mdelay is not good
> > idea as it is okay to sleep shorter time at this particular place.
> 
> First, just to make sure we are on the same page, the replacement is
> with msleep(), not mdelay(). The difference between the two is that
> msleep() uses schedule_timeout() to give up the CPU while mdelay() busy
> waits.

Ahha, I did not notice that. msleep() is okay.
								Pavel

-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: [PATCH] nbd: replace schedule_timeout()

[Felipe W Damasio]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

	Hi,

Nishanth Aravamudan wrote:
> After discussing with Greg, longer delays were considered those
> measurable in milliseconds. I think msleep() helps add a layer of
> consistency across the board. It also is (IMO) easier to think of times
> and delays in terms of seconds / milliseconds rather than jiffies (this
> becomes very apparent in later patches, where all sorts of crazy
> conversions are replaced with simple numbers).

	Yeah, you're right, msleep seems to be the right way to go.

	Cheers,

Felipe
-- 
It's most certainly GNU/Linux, not Linux. Read more at
http://www.gnu.org/gnu/why-gnu-linux.html

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors