[KJ] RE: [PATCH 3/22] block/cciss: replace schedule_timeout() with

[KJ] RE: [PATCH 3/22] block/cciss: replace schedule_timeout() with

[Miller, Mike (OS Dev)]

> From: Nishanth Aravamudan [mailto:nacc@us.ibm.com]
> Hi,
> 
> Please consider applying.
> 
> I used msleep(10) here under the presumption that the 
> schedule_timeout(1) was
> written assuming that HZ\x100 (as it used to be), which is
> equivalent to 10 milliseconds. If the desire is actually for 
> 1 ms or the minimal
> sleep interval, then the patch can be changed appropriately. 
> A similar assumption
> as to the constant delay value was made in the other 
> replacement, which can also
> be appropriately adjusted.
> 
> Description: Change the delay logic in pollcomplete() to use 
> msleep() and
> time_before(). Instead of assuming schedule_timeout() will 
> sleep exactly as
> requested, use msleep(10) to guarantee minimally 10 
> millisecond increments and
> time_before() to guarantee stopping the loop as close to 20 
> seconds as possible.
> Also changes another occurrence of schedule_timeout() to msleep().
> TASK_INTERRUPTIBLE is used in this case, but signals are not handled. 
> 
> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>

I still fail to see the benefits of this patch.

mikem

> 
> --- 2.6.11-rc1-kj-v/drivers/block/cciss.c	2005-01-15 
> 16:55:43.000000000 -0800
> +++ 2.6.11-rc1-kj/drivers/block/cciss.c	2005-01-17 
> 10:33:44.000000000 -0800
> @@ -1729,17 +1729,15 @@ static int cciss_revalidate(struct gendi
>  static unsigned long pollcomplete(int ctlr)
>  {
>  	unsigned long done;
> -	int i;
> +	unsigned long end_jiffies = jiffies + 20 * HZ;
>  
>  	/* Wait (up to 20 seconds) for a command to complete */
> -
> -	for (i = 20 * HZ; i > 0; i--) {
> +	while (time_before(jiffies,end_jiffies)) {
>  		done = hba[ctlr]->access.command_completed(hba[ctlr]);
> -		if (done = FIFO_EMPTY) {
> -			set_current_state(TASK_UNINTERRUPTIBLE);
> -			schedule_timeout(1);
> -		} else
> -			return (done);
> +		if (done = FIFO_EMPTY)
> +			msleep(10);
> +		else
> +			return done;
>  	}
>  	/* Invalid address to tell caller we ran out of time */
>  	return 1;
> @@ -2512,8 +2510,7 @@ static int cciss_pci_init(ctlr_info_t *c
>  		if (!(readl(c->vaddr + SA5_DOORBELL) & 
> CFGTBL_ChangeReq))
>  			break;
>  		/* delay and try again */
> -		set_current_state(TASK_INTERRUPTIBLE);
> -		schedule_timeout(10);
> +		msleep(100);
>  	}	
>  
>  #ifdef CCISS_DEBUG
> 

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

Re: [KJ] RE: [PATCH 3/22] block/cciss: replace schedule_timeout()

[Greg KH]

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

On Mon, Jan 17, 2005 at 04:14:10PM -0600, Miller, Mike (OS Dev) wrote:
> > From: Nishanth Aravamudan [mailto:nacc@us.ibm.com]
> > Hi,
> > 
> > Please consider applying.
> > 
> > I used msleep(10) here under the presumption that the 
> > schedule_timeout(1) was
> > written assuming that HZ=100 (as it used to be), which is
> > equivalent to 10 milliseconds. If the desire is actually for 
> > 1 ms or the minimal
> > sleep interval, then the patch can be changed appropriately. 
> > A similar assumption
> > as to the constant delay value was made in the other 
> > replacement, which can also
> > be appropriately adjusted.
> > 
> > Description: Change the delay logic in pollcomplete() to use 
> > msleep() and
> > time_before(). Instead of assuming schedule_timeout() will 
> > sleep exactly as
> > requested, use msleep(10) to guarantee minimally 10 
> > millisecond increments and
> > time_before() to guarantee stopping the loop as close to 20 
> > seconds as possible.
> > Also changes another occurrence of schedule_timeout() to msleep().
> > TASK_INTERRUPTIBLE is used in this case, but signals are not handled. 
> > 
> > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> 
> I still fail to see the benefits of this patch.

When HZ changed from 100 to 1000, your code went from sleeping 10ms to 1
ms.  Did you really mean that?  This way, your code always sleeps what
you expect it to sleep, no matter what the value of HZ.

thanks,

greg k-h


[-- 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

[KJ] Re: [PATCH 3/22] block/cciss: replace schedule_timeout() with

[Nishanth Aravamudan]

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

On Mon, Jan 17, 2005 at 04:14:10PM -0600, Miller, Mike (OS Dev) wrote:
> > From: Nishanth Aravamudan [mailto:nacc@us.ibm.com]
> > Hi,
> > 
> > Please consider applying.
> > 
> > I used msleep(10) here under the presumption that the 
> > schedule_timeout(1) was
> > written assuming that HZ=100 (as it used to be), which is
> > equivalent to 10 milliseconds. If the desire is actually for 
> > 1 ms or the minimal
> > sleep interval, then the patch can be changed appropriately. 
> > A similar assumption
> > as to the constant delay value was made in the other 
> > replacement, which can also
> > be appropriately adjusted.
> > 
> > Description: Change the delay logic in pollcomplete() to use 
> > msleep() and
> > time_before(). Instead of assuming schedule_timeout() will 
> > sleep exactly as
> > requested, use msleep(10) to guarantee minimally 10 
> > millisecond increments and
> > time_before() to guarantee stopping the loop as close to 20 
> > seconds as possible.
> > Also changes another occurrence of schedule_timeout() to msleep().
> > TASK_INTERRUPTIBLE is used in this case, but signals are not handled. 
> > 
> > Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> 
> I still fail to see the benefits of this patch.

There is a clear benefit (IMO) to use actual time units for sleeps (msleep()
and co. use milliseconds or seconds) as opposed to jiffy-relative units (as
in schedule_timeout() where you are requesting a delay in jiffies, which
varies from arch to arch).

This becomes very important with dynamic HZ, for instance. Clearly if HZ
changes, then the delay caused by schedule_timeout(1) will change, which is not
necessarily desired and may be confusing the user. A time specified in real
time units will be adjusted with the change in HZ.

If it is not important how long (as long as it is not too long) the driver
sleeps, then I believe msleep() should be preferred in effectively all cases
(depending on whether wait-queue events or signals may be important early
triggers, of course). Milliseconds indicate a clear delay, independent of
HZ's value. Jiffy delays have a clear reliance on the value of HZ, but using
constant delays with schedule_timeout (1, 10, etc., i.e. values that are
independent of the value of HZ (examples of the latter would be HZ/2, HZ/4,
etc.)) masks this dependancy which may be confusing to the user who sees
dramatic changes in the delay of particular drivers as HZ is altered.

I am open to other suggestions, but I think these are good reasons. Another
basic one is that it will lead to consistency across the board. HZ is not a
measure of time, it should not be used as a measure of time. Instead,
milliseconds should be used and, when/if the facility has been added,
microseconds and nanoseconds.

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: [KJ] RE: [PATCH 3/22] block/cciss: replace schedule_timeout()

[Miller, Mike (OS Dev)]

> From: Greg KH [mailto:greg@kroah.com]
> Subject: Re: [KJ] RE: [PATCH 3/22] block/cciss: replace
> schedule_timeout() with msleep()
> 
> 
> On Mon, Jan 17, 2005 at 04:14:10PM -0600, Miller, Mike (OS Dev) wrote:
> > I still fail to see the benefits of this patch.
> 
> When HZ changed from 100 to 1000, your code went from 
> sleeping 10ms to 1
> ms.  Did you really mean that?  This way, your code always sleeps what
> you expect it to sleep, no matter what the value of HZ.
> 
> thanks,
> 
> greg k-h

I went back thru the code.

Acked-by: Mike Miller <mike.miller@hp.com>


> 
> 

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