RE: [patch 08/10] scsi/dpt_i2o: replace schedule_timeout()withmsleep_interruptible()

RE: [patch 08/10] scsi/dpt_i2o: replace schedule_timeout()withmsleep_interruptible()

[Salyzyn, Mark]

I have to speak in absence of the original author, but I believe the
decision to conditionalize the unlock was because this routine is called
at init time and at run time.

I did not notice the `stutter' of the two schedule_timeout's. Eeeeek!!!
(My scientific replacement for bogus). Thanks for shaking the cobwebs.

The second schedule_timeout needs to be *removed*, it does not belong
there at all!

Sincerely -- Mark Salyzyn

-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@SteelEye.com] 
Sent: Thursday, October 21, 2004 9:47 AM
To: Salyzyn, Mark
Cc: janitor@sternwelten.at; SCSI Mailing List; nacc@us.ibm.com
Subject: RE: [patch 08/10] scsi/dpt_i2o: replace
schedule_timeout()withmsleep_interruptible()

On Thu, 2004-10-21 at 09:17, Salyzyn, Mark wrote:
> The driver does set the state to TASK_INTERRUPTIBLE a handful of lines
> above, is there some other state that the driver should have set?

No, it should be set to TASK_[UN]INTERRUPTIBLE.  However, you can't just
do it once.  When the schedule_timeout() returns, the state is once more
TASK_RUNNING.  If you want to do another schedule_timeout() like you're
doing in this case, you have to set it to TASK_[UN]INTERRUPTIBLE again.

> We should remove the set_current_state(TASK_INTERRUPTIBLE) line when
> replacing with the msleep_interruptible(timeout * 1000) line.

Actually, the whole thing:

	if((status = adpt_i2o_post_this(pHba, msg, len)) == 0){
		set_current_state(TASK_INTERRUPTIBLE);
		if(pHba->host)
			spin_unlock_irq(pHba->host->host_lock);
		if (!timeout)
			schedule();
		else{
			timeout = schedule_timeout(timeout);
			if (timeout == 0) {
				// I/O issued, but cannot get result in
				// specified time. Freeing resorces is
				// dangerous.
				status = -ETIME;
			}
			schedule_timeout(timeout*HZ);
		}
		if(pHba->host)
			spin_lock_irq(pHba->host->host_lock);
	}

Looks a bit bogus.  Are the conditional spinlocks really necesary? Why
is timeout multiplied by HZ in the second case, but not in the first
case?  The only reason why timeout would be non-zero in the first
schedule timeout would be if the sleep were interrupted.

James

Re: [patch 08/10] scsi/dpt_i2o: replace schedule_timeout()withmsleep_interruptible()

[Nishanth Aravamudan]

On Thu, Oct 21, 2004 at 10:11:31AM -0400, Salyzyn, Mark wrote:
> I have to speak in absence of the original author, but I believe the
> decision to conditionalize the unlock was because this routine is called
> at init time and at run time.
> 
> I did not notice the `stutter' of the two schedule_timeout's. Eeeeek!!!
> (My scientific replacement for bogus). Thanks for shaking the cobwebs.
> 
> The second schedule_timeout needs to be *removed*, it does not belong
> there at all!

Please find below a patch which does exactly this.

Description: Removes a schedule_timeout() from adpt_i2o_post_wait() whch
Mark Salyzyn requested be removed.

Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>


--- 2.6.9-bk7-vanilla/drivers/scsi/dpt_i2o.c	2004-10-22 10:41:29.000000000 -0700
+++ 2.6.9-bk7/drivers/scsi/dpt_i2o.c	2004-10-22 15:22:53.000000000 -0700
@@ -1179,7 +1179,6 @@ static int adpt_i2o_post_wait(adpt_hba* 
 				// dangerous.
 				status = -ETIME;
 			}
-			schedule_timeout(timeout*HZ);
 		}
 		if(pHba->host)
 			spin_lock_irq(pHba->host->host_lock);

Re: [patch 08/10] scsi/dpt_i2o: replace schedule_timeout()withmsleep_interruptible()

[maximilian attems]

On Fri, 22 Oct 2004, Nishanth Aravamudan wrote:

> On Thu, Oct 21, 2004 at 10:11:31AM -0400, Salyzyn, Mark wrote:
> > I have to speak in absence of the original author, but I believe the
> > decision to conditionalize the unlock was because this routine is called
> > at init time and at run time.
> > 
> > I did not notice the `stutter' of the two schedule_timeout's. Eeeeek!!!
> > (My scientific replacement for bogus). Thanks for shaking the cobwebs.
> > 
> > The second schedule_timeout needs to be *removed*, it does not belong
> > there at all!
> 
> Please find below a patch which does exactly this.
> 
> Description: Removes a schedule_timeout() from adpt_i2o_post_wait() whch
> Mark Salyzyn requested be removed.
 
this patch seems already applied in 2.6.10-rc1.
please double check.

thanks maks

Re: [patch 08/10] scsi/dpt_i2o: replace schedule_timeout()withmsleep_interruptible()

[James Bottomley]

On Sun, 2004-10-24 at 10:03, maximilian attems wrote:
> this patch seems already applied in 2.6.10-rc1.
> please double check.

I don't think it is.  However we're not finished with shaking the
cobwebs out of this routine yet.

This:

               set_current_state(TASK_INTERRUPTIBLE);
		[...]
                if (!timeout)
                        schedule();
 
Would sleep forever if timeout were zero because nothing ever seems to
shift the task back into TASK_RUNNING (i.e. wake it up).

Anyway, I think the attached looks to be the best way to fix all the
issues.

James

===== include/linux/delay.h 1.6 vs edited =====
--- 1.6/include/linux/delay.h	2004-09-03 05:08:32 -04:00
+++ edited/include/linux/delay.h	2004-10-24 10:38:09 -04:00
@@ -46,4 +46,12 @@
 	msleep(seconds * 1000);
 }
 
+static inline unsigned long ssleep_interruptible(unsigned int seconds)
+{
+	unsigned long ret = msleep_interruptible(seconds * 1000);
+	/* Round up to seconds for return (i.e 0.001s remaining
+	 * becomes 1s) */
+	return ret/1000 + (ret ? 1 : 0);
+}
+
 #endif /* defined(_LINUX_DELAY_H) */
===== drivers/scsi/dpt_i2o.c 1.44 vs edited =====
--- 1.44/drivers/scsi/dpt_i2o.c	2004-07-26 18:03:34 -04:00
+++ edited/drivers/scsi/dpt_i2o.c	2004-10-24 10:34:17 -04:00
@@ -1164,22 +1164,14 @@
 	spin_unlock(&adpt_wq_i2o_post.lock);
 
 	msg[2] |= 0x80000000 | ((u32)wait_data->id);
-	timeout *= HZ;
 	if((status = adpt_i2o_post_this(pHba, msg, len)) == 0){
-		set_current_state(TASK_INTERRUPTIBLE);
 		if(pHba->host)
 			spin_unlock_irq(pHba->host->host_lock);
-		if (!timeout)
-			schedule();
-		else{
-			timeout = schedule_timeout(timeout);
-			if (timeout == 0) {
-				// I/O issued, but cannot get result in
-				// specified time. Freeing resorces is
-				// dangerous.
-				status = -ETIME;
-			}
-			schedule_timeout(timeout*HZ);
+		if (ssleep_interruptible(timeout)) {
+			// I/O issued, but cannot get result in
+			// specified time. Freeing resorces is
+			// dangerous.
+			status = -ETIME;
 		}
 		if(pHba->host)
 			spin_lock_irq(pHba->host->host_lock);

RE: [patch 08/10] scsi/dpt_i2o: replace schedule_timeout()withmsleep_interruptible()

[Salyzyn, Mark]

Not quite ... The value of 0 is meant to trigger never timing out on the
request (#define FOREVER (0) in dpti.h). The code change you have will
mean that the code will immediately timeout, not the intent.

There is a wakeup to the thread issued by the interrupt via a WAIT
QUEUE, if the sequence of TASK_INTERRUPTIBLE/schedule() did not work, it
would not explain the successful operation of the driver during
initialization time.

At this point, I like the simpler patch from Nishanth Aravamudan :-)

Sincerely -- Mark Salyzyn

-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@SteelEye.com] 
Sent: Sunday, October 24, 2004 10:40 AM
To: maximilian attems
Cc: Nishanth Aravamudan; Salyzyn, Mark; SCSI Mailing List
Subject: Re: [patch 08/10] scsi/dpt_i2o: replace
schedule_timeout()withmsleep_interruptible()

On Sun, 2004-10-24 at 10:03, maximilian attems wrote:
> this patch seems already applied in 2.6.10-rc1.
> please double check.

I don't think it is.  However we're not finished with shaking the
cobwebs out of this routine yet.

This:

               set_current_state(TASK_INTERRUPTIBLE);
		[...]
                if (!timeout)
                        schedule();
 
Would sleep forever if timeout were zero because nothing ever seems to
shift the task back into TASK_RUNNING (i.e. wake it up).

Anyway, I think the attached looks to be the best way to fix all the
issues.

James

===== include/linux/delay.h 1.6 vs edited =====
--- 1.6/include/linux/delay.h	2004-09-03 05:08:32 -04:00
+++ edited/include/linux/delay.h	2004-10-24 10:38:09 -04:00
@@ -46,4 +46,12 @@
 	msleep(seconds * 1000);
 }
 
+static inline unsigned long ssleep_interruptible(unsigned int seconds)
+{
+	unsigned long ret = msleep_interruptible(seconds * 1000);
+	/* Round up to seconds for return (i.e 0.001s remaining
+	 * becomes 1s) */
+	return ret/1000 + (ret ? 1 : 0);
+}
+
 #endif /* defined(_LINUX_DELAY_H) */
===== drivers/scsi/dpt_i2o.c 1.44 vs edited =====
--- 1.44/drivers/scsi/dpt_i2o.c	2004-07-26 18:03:34 -04:00
+++ edited/drivers/scsi/dpt_i2o.c	2004-10-24 10:34:17 -04:00
@@ -1164,22 +1164,14 @@
 	spin_unlock(&adpt_wq_i2o_post.lock);
 
 	msg[2] |= 0x80000000 | ((u32)wait_data->id);
-	timeout *= HZ;
 	if((status = adpt_i2o_post_this(pHba, msg, len)) == 0){
-		set_current_state(TASK_INTERRUPTIBLE);
 		if(pHba->host)
 			spin_unlock_irq(pHba->host->host_lock);
-		if (!timeout)
-			schedule();
-		else{
-			timeout = schedule_timeout(timeout);
-			if (timeout == 0) {
-				// I/O issued, but cannot get result in
-				// specified time. Freeing resorces is
-				// dangerous.
-				status = -ETIME;
-			}
-			schedule_timeout(timeout*HZ);
+		if (ssleep_interruptible(timeout)) {
+			// I/O issued, but cannot get result in
+			// specified time. Freeing resorces is
+			// dangerous.
+			status = -ETIME;
 		}
 		if(pHba->host)
 			spin_lock_irq(pHba->host->host_lock);

RE: [patch 08/10] scsi/dpt_i2o: replace schedule_timeout()withmsleep_interruptible()

[James Bottomley]

On Mon, 2004-10-25 at 07:30, Salyzyn, Mark wrote:
> Not quite ... The value of 0 is meant to trigger never timing out on the
> request (#define FOREVER (0) in dpti.h). The code change you have will
> mean that the code will immediately timeout, not the intent.
> 
> There is a wakeup to the thread issued by the interrupt via a WAIT
> QUEUE, if the sequence of TASK_INTERRUPTIBLE/schedule() did not work, it
> would not explain the successful operation of the driver during
> initialization time.

So this is basically an open coded interruptible_sleep_on_timeout()? 
Why not replace it with this? (although this will excite the janitors
since sleep_on is deprecated in all its forms).

Also, if you define your FOREVER to be MAX_SCHEDULE_TIMEOUT, you don't
need to special case this.

James

RE: [patch 08/10] scsi/dpt_i2o: replace schedule_timeout() withmsleep_interruptible()

[Salyzyn, Mark]

The driver does set the state to TASK_INTERRUPTIBLE a handful of lines
above, is there some other state that the driver should have set?

We should remove the set_current_state(TASK_INTERRUPTIBLE) line when
replacing with the msleep_interruptible(timeout * 1000) line.

Sincerely -- Mark Salyzyn

-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@SteelEye.com] 
Sent: Thursday, October 21, 2004 8:51 AM
To: Salyzyn, Mark
Cc: janitor@sternwelten.at; SCSI Mailing List; nacc@us.ibm.com
Subject: RE: [patch 08/10] scsi/dpt_i2o: replace schedule_timeout()
withmsleep_interruptible()

On Thu, 2004-10-21 at 07:44, Salyzyn, Mark wrote:
> The timeout granularity of 1 second should be the first clue that this
> is not important. Commands complete and wake up the task under normal
> operation; the timeout is to deal with an errant controller.

Erm, yes, but what you're doing is clearly incorrect, and the reason we
went to msleep variants anyway.  Your code forgets to set the current
state, so you stand a good chance of returning immediately from the
schedule_timeout() without even yielding the processor.

James

RE: [patch 08/10] scsi/dpt_i2o: replace schedule_timeout() withmsleep_interruptible()

[James Bottomley]

On Thu, 2004-10-21 at 09:17, Salyzyn, Mark wrote:
> The driver does set the state to TASK_INTERRUPTIBLE a handful of lines
> above, is there some other state that the driver should have set?

No, it should be set to TASK_[UN]INTERRUPTIBLE.  However, you can't just
do it once.  When the schedule_timeout() returns, the state is once more
TASK_RUNNING.  If you want to do another schedule_timeout() like you're
doing in this case, you have to set it to TASK_[UN]INTERRUPTIBLE again.

> We should remove the set_current_state(TASK_INTERRUPTIBLE) line when
> replacing with the msleep_interruptible(timeout * 1000) line.

Actually, the whole thing:

	if((status = adpt_i2o_post_this(pHba, msg, len)) == 0){
		set_current_state(TASK_INTERRUPTIBLE);
		if(pHba->host)
			spin_unlock_irq(pHba->host->host_lock);
		if (!timeout)
			schedule();
		else{
			timeout = schedule_timeout(timeout);
			if (timeout == 0) {
				// I/O issued, but cannot get result in
				// specified time. Freeing resorces is
				// dangerous.
				status = -ETIME;
			}
			schedule_timeout(timeout*HZ);
		}
		if(pHba->host)
			spin_lock_irq(pHba->host->host_lock);
	}

Looks a bit bogus.  Are the conditional spinlocks really necesary? Why
is timeout multiplied by HZ in the second case, but not in the first
case?  The only reason why timeout would be non-zero in the first
schedule timeout would be if the sleep were interrupted.

James