[Kernel-janitors] [PATCH 2.6.9-rc2 2/33] char/cyclades: replace

[Kernel-janitors] [PATCH 2.6.9-rc2 2/33] char/cyclades: replace

[Nishanth Aravamudan]

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

Any comments would be appreciated. 

Description: Use msleep_interruptible() instead of schedule_timeout()
to guarantee the task delays as expected.

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

--- 2.6.9-rc2-vanilla/drivers/char/cyclades.c	2004-09-13 17:15:49.000000000 -0700
+++ 2.6.9-rc2/drivers/char/cyclades.c	2004-09-15 14:20:23.000000000 -0700
@@ -2717,8 +2717,7 @@ cy_wait_until_sent(struct tty_struct *tt
 #ifdef CY_DEBUG_WAIT_UNTIL_SENT
 	    printk("Not clean (jiff=%lu)...", jiffies);
 #endif
-	    current->state = TASK_INTERRUPTIBLE;
-	    schedule_timeout(char_time);
+	    msleep_interruptible(msecs_to_jiffies(char_time));
 	    if (signal_pending(current))
 		break;
 	    if (timeout && time_after(jiffies, orig_jiffies + timeout))
@@ -2729,8 +2728,7 @@ cy_wait_until_sent(struct tty_struct *tt
 	// Nothing to do!
     }
     /* Run one more char cycle */
-    current->state = TASK_INTERRUPTIBLE;
-    schedule_timeout(char_time * 5);
+    msleep_interruptible(jiffies_to_msecs(char_time * 5));
 #ifdef CY_DEBUG_WAIT_UNTIL_SENT
     printk("Clean (jiff=%lu)...done\n", jiffies);
 #endif
@@ -2860,8 +2858,7 @@ cy_close(struct tty_struct *tty, struct 
     if (info->blocked_open) {
 	CY_UNLOCK(info, flags);
         if (info->close_delay) {
-            current->state = TASK_INTERRUPTIBLE;
-            schedule_timeout(info->close_delay);
+            msleep_interruptible(jiffies_to_msecs(info->close_delay));
         }
         wake_up_interruptible(&info->open_wait);
 	CY_LOCK(info, flags);

[-- 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] [PATCH 2.6.9-rc2 2/33] char/cyclades: replace

[Nishanth Aravamudan]

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

On Wed, Sep 15, 2004 at 02:21:55PM -0700, Nishanth Aravamudan wrote:
> Any comments would be appreciated. 
> 
> Description: Use msleep_interruptible() instead of schedule_timeout()
> to guarantee the task delays as expected.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> 
> --- 2.6.9-rc2-vanilla/drivers/char/cyclades.c	2004-09-13 17:15:49.000000000 -0700
> +++ 2.6.9-rc2/drivers/char/cyclades.c	2004-09-15 14:20:23.000000000 -0700
> @@ -2717,8 +2717,7 @@ cy_wait_until_sent(struct tty_struct *tt
>  #ifdef CY_DEBUG_WAIT_UNTIL_SENT
>  	    printk("Not clean (jiff=%lu)...", jiffies);
>  #endif
> -	    current->state = TASK_INTERRUPTIBLE;
> -	    schedule_timeout(char_time);
> +	    msleep_interruptible(msecs_to_jiffies(char_time));

The original patch mistakenly used the msecs_to_jiffies() macro when it
should have used the jiffies_to_msecs() macro. Please find the corrected
patch below.


--- 2.6.9-rc2-vanilla/drivers/char/cyclades.c	2004-09-13 17:15:49.000000000 -0700
+++ 2.6.9-rc2/drivers/char/cyclades.c	2004-09-24 10:23:05.000000000 -0700
@@ -2717,8 +2717,7 @@ cy_wait_until_sent(struct tty_struct *tt
 #ifdef CY_DEBUG_WAIT_UNTIL_SENT
 	    printk("Not clean (jiff=%lu)...", jiffies);
 #endif
-	    current->state = TASK_INTERRUPTIBLE;
-	    schedule_timeout(char_time);
+	    msleep_interruptible(jiffies_to_msecs(char_time));
 	    if (signal_pending(current))
 		break;
 	    if (timeout && time_after(jiffies, orig_jiffies + timeout))
@@ -2729,8 +2728,7 @@ cy_wait_until_sent(struct tty_struct *tt
 	// Nothing to do!
     }
     /* Run one more char cycle */
-    current->state = TASK_INTERRUPTIBLE;
-    schedule_timeout(char_time * 5);
+    msleep_interruptible(jiffies_to_msecs(char_time * 5));
 #ifdef CY_DEBUG_WAIT_UNTIL_SENT
     printk("Clean (jiff=%lu)...done\n", jiffies);
 #endif
@@ -2860,8 +2858,7 @@ cy_close(struct tty_struct *tty, struct 
     if (info->blocked_open) {
 	CY_UNLOCK(info, flags);
         if (info->close_delay) {
-            current->state = TASK_INTERRUPTIBLE;
-            schedule_timeout(info->close_delay);
+            msleep_interruptible(jiffies_to_msecs(info->close_delay));
         }
         wake_up_interruptible(&info->open_wait);
 	CY_LOCK(info, flags);

[-- 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] [PATCH 2.6.9-rc2 2/33] char/cyclades: replace

[Marcelo Tosatti]

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


Nishanth,


Well have to convert from jiffies (char_time) to msecs, just to 
convert it back again to jiffies in msleep_interruptible().

Is this patch actually fixing any problem?

Thanks and sorry for the long delay

> The original patch mistakenly used the msecs_to_jiffies() macro when it
> should have used the jiffies_to_msecs() macro. Please find the corrected
> patch below.
> 
> 
> --- 2.6.9-rc2-vanilla/drivers/char/cyclades.c	2004-09-13 17:15:49.000000000 -0700
> +++ 2.6.9-rc2/drivers/char/cyclades.c	2004-09-24 10:23:05.000000000 -0700
> @@ -2717,8 +2717,7 @@ cy_wait_until_sent(struct tty_struct *tt
>  #ifdef CY_DEBUG_WAIT_UNTIL_SENT
>  	    printk("Not clean (jiff=%lu)...", jiffies);
>  #endif
> -	    current->state = TASK_INTERRUPTIBLE;
> -	    schedule_timeout(char_time);
> +	    msleep_interruptible(jiffies_to_msecs(char_time));
>  	    if (signal_pending(current))
>  		break;
>  	    if (timeout && time_after(jiffies, orig_jiffies + timeout))
> @@ -2729,8 +2728,7 @@ cy_wait_until_sent(struct tty_struct *tt
>  	// Nothing to do!
>      }
>      /* Run one more char cycle */
> -    current->state = TASK_INTERRUPTIBLE;
> -    schedule_timeout(char_time * 5);
> +    msleep_interruptible(jiffies_to_msecs(char_time * 5));
>  #ifdef CY_DEBUG_WAIT_UNTIL_SENT
>      printk("Clean (jiff=%lu)...done\n", jiffies);
>  #endif
> @@ -2860,8 +2858,7 @@ cy_close(struct tty_struct *tty, struct 
>      if (info->blocked_open) {
>  	CY_UNLOCK(info, flags);
>          if (info->close_delay) {
> -            current->state = TASK_INTERRUPTIBLE;
> -            schedule_timeout(info->close_delay);
> +            msleep_interruptible(jiffies_to_msecs(info->close_delay));
>          }
>          wake_up_interruptible(&info->open_wait);
>  	CY_LOCK(info, flags);

[-- 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] [PATCH 2.6.9-rc2 2/33] char/cyclades: replace

[Marcelo Tosatti]

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

On Fri, Oct 01, 2004 at 01:10:31PM -0700, Nishanth Aravamudan wrote:
> On Thu, Sep 30, 2004 at 01:48:18PM -0300, Marcelo Tosatti wrote:
> > Well have to convert from jiffies (char_time) to msecs, just to 
> > convert it back again to jiffies in msleep_interruptible().
> > 
> > Is this patch actually fixing any problem?
> 
> So there are a few things here:
> 
> 1) I think the overhead of the conversions is very small in comparison
> with the eventual call to schedule() and such, so I don't think it's
> that big of a deal -- *please* correct me if I am wrong, though.

No you are right the overhead is very small - I'm just trying
to find a good reason to apply the change.

> 2) I am currently trying to determine if it might be possible for the
> timeout value (char_time, and I guess by extension, timeout) to be
> expressed in milliseconds instead of jiffies. If you have any input on
> that, Marcelo (or anyone from the list), I would really appreciate it.

I do not like the idea very much. 

I mean, what is the problem with that piece of code right now?

ie please educate me about msleep_interruptible()



[-- 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] [PATCH 2.6.9-rc2 2/33] char/cyclades: replace

[Nishanth Aravamudan]

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

On Thu, Sep 30, 2004 at 01:48:18PM -0300, Marcelo Tosatti wrote:
> Well have to convert from jiffies (char_time) to msecs, just to 
> convert it back again to jiffies in msleep_interruptible().
> 
> Is this patch actually fixing any problem?

So there are a few things here:

1) I think the overhead of the conversions is very small in comparison
with the eventual call to schedule() and such, so I don't think it's
that big of a deal -- *please* correct me if I am wrong, though.

2) I am currently trying to determine if it might be possible for the
timeout value (char_time, and I guess by extension, timeout) to be
expressed in milliseconds instead of jiffies. If you have any input on
that, Marcelo (or anyone from the list), I would really appreciate it.

-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] [PATCH 2.6.9-rc2 2/33] char/cyclades: replace

[Nishanth Aravamudan]

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

On Fri, Oct 01, 2004 at 04:07:04PM -0300, Marcelo Tosatti wrote:
> On Fri, Oct 01, 2004 at 01:10:31PM -0700, Nishanth Aravamudan wrote:
> > On Thu, Sep 30, 2004 at 01:48:18PM -0300, Marcelo Tosatti wrote:
> > > Well have to convert from jiffies (char_time) to msecs, just to 
> > > convert it back again to jiffies in msleep_interruptible().
> > > 
> > > Is this patch actually fixing any problem?
> > 
> > So there are a few things here:
> > 
> > 1) I think the overhead of the conversions is very small in comparison
> > with the eventual call to schedule() and such, so I don't think it's
> > that big of a deal -- *please* correct me if I am wrong, though.
> 
> No you are right the overhead is very small - I'm just trying
> to find a good reason to apply the change.

So I would say there are some good reasons to apply this patch.
Presuming this delay is somewhat long (it's a little hard to determine
this from the code, so your input here would again be appreciated),
having most drivers delay for long times using msleep*() goes a long way
towards consistency IMO. Since your timeout values are variables,
though, this becomes a little more difficult to argue :) Then again, if
we could figure out a way to use seconds (or some other human time-unit)
to express the delays, I think using msleep*() becomes pretty obvious.
Or even if it doesn't happen now for whatever reason, down the road this
makes the change that much easier

> > 2) I am currently trying to determine if it might be possible for the
> > timeout value (char_time, and I guess by extension, timeout) to be
> > expressed in milliseconds instead of jiffies. If you have any input on
> > that, Marcelo (or anyone from the list), I would really appreciate it.
> 
> I do not like the idea very much. 

Out of curiousity, why? Would it be particularly hard to do? If that is
the case, that's fine, I'm just wondering...

> I mean, what is the problem with that piece of code right now?
> 
> ie please educate me about msleep_interruptible()

To be honest, I think the arguments for using msleep() in general are
far stronger. msleep_interruptible() was recently introduced to have the
same effect except allow for a TASK_INTERRUPTIBLE based sleep. Since you
are checking for a signal after returning from schedule_timeout(),
msleep() isn't an option here ... What can be done here, actually, I
just realized is to remove the signal_pending() check, as
msleep_interruptible() does this as well.

There is another thing my patch didn't do, though - the

	current->state = TASK_RUNNING;

call is unnecessary, because schedule_timeout() / msleep_interruptible()
takes care of this.

I have made these changes in the following patch -- please revert the
previous cyclades patch and apply this one. Let me know if this is more
satisfactory (it definitely simplifies the while-loop a bit).

-Nish



Description: Uses msleep_interruptible() instead of schedule_timeout().
This allows the removal of one conditional, as msleep_interruptible()
should only return non-zero if signal_pending(current) is true.
current->state also does not need to be set, as msleep_interruptible()
(actually, schedule_timeout) is guaranteed to return in TASK_RUNNING.

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

--- 2.6.9-rc2-vanilla/drivers/char/cyclades.c	2004-09-13 17:15:49.000000000 -0700
+++ 2.6.9-rc2/drivers/char/cyclades.c	2004-10-01 13:55:19.000000000 -0700
@@ -2717,20 +2717,16 @@ cy_wait_until_sent(struct tty_struct *tt
 #ifdef CY_DEBUG_WAIT_UNTIL_SENT
 	    printk("Not clean (jiff=%lu)...", jiffies);
 #endif
-	    current->state = TASK_INTERRUPTIBLE;
-	    schedule_timeout(char_time);
-	    if (signal_pending(current))
+	    if (msleep_interruptible(jiffies_to_msecs(char_time)))
 		break;
 	    if (timeout && time_after(jiffies, orig_jiffies + timeout))
 		break;
 	}
-	current->state = TASK_RUNNING;
     } else {
 	// Nothing to do!
     }
     /* Run one more char cycle */
-    current->state = TASK_INTERRUPTIBLE;
-    schedule_timeout(char_time * 5);
+    msleep_interruptible(jiffies_to_msecs(char_time * 5));
 #ifdef CY_DEBUG_WAIT_UNTIL_SENT
     printk("Clean (jiff=%lu)...done\n", jiffies);
 #endif
@@ -2860,8 +2856,7 @@ cy_close(struct tty_struct *tty, struct 
     if (info->blocked_open) {
 	CY_UNLOCK(info, flags);
         if (info->close_delay) {
-            current->state = TASK_INTERRUPTIBLE;
-            schedule_timeout(info->close_delay);
+            msleep_interruptible(jiffies_to_msecs(info->close_delay));
         }
         wake_up_interruptible(&info->open_wait);
 	CY_LOCK(info, flags);

[-- 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] [PATCH 2.6.9-rc2 2/33] char/cyclades: replace

[Marcelo Tosatti]

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

On Fri, Oct 01, 2004 at 02:09:01PM -0700, Nishanth Aravamudan wrote:
> On Fri, Oct 01, 2004 at 04:07:04PM -0300, Marcelo Tosatti wrote:
> > On Fri, Oct 01, 2004 at 01:10:31PM -0700, Nishanth Aravamudan wrote:
> > > On Thu, Sep 30, 2004 at 01:48:18PM -0300, Marcelo Tosatti wrote:
> > > > Well have to convert from jiffies (char_time) to msecs, just to 
> > > > convert it back again to jiffies in msleep_interruptible().
> > > > 
> > > > Is this patch actually fixing any problem?
> > > 
> > > So there are a few things here:
> > > 
> > > 1) I think the overhead of the conversions is very small in comparison
> > > with the eventual call to schedule() and such, so I don't think it's
> > > that big of a deal -- *please* correct me if I am wrong, though.
> > 
> > No you are right the overhead is very small - I'm just trying
> > to find a good reason to apply the change.
> 
> So I would say there are some good reasons to apply this patch.
> Presuming this delay is somewhat long (it's a little hard to determine
> this from the code, so your input here would again be appreciated),
> having most drivers delay for long times using msleep*() goes a long way
> towards consistency IMO. Since your timeout values are variables,
> though, this becomes a little more difficult to argue :) 

The value will never be more than 1 second.

> Then again, if we could figure out a way to use seconds (or some other human time-unit)
> to express the delays, I think using msleep*() becomes pretty obvious.
> Or even if it doesn't happen now for whatever reason, down the road this
> makes the change that much easier
> 
> > > 2) I am currently trying to determine if it might be possible for the
> > > timeout value (char_time, and I guess by extension, timeout) to be
> > > expressed in milliseconds instead of jiffies. If you have any input on
> > > that, Marcelo (or anyone from the list), I would really appreciate it.
> > 
> > I do not like the idea very much. 
> 
> Out of curiousity, why? Would it be particularly hard to do? If that is
> the case, that's fine, I'm just wondering...

No it woudlnt be very hard to do it. change "timeout" from cyclades_port
structure from jiffies to seconds. 

I just dont see a point in doing it right now (I might be a good thing, though).

> > I mean, what is the problem with that piece of code right now?
> > 
> > ie please educate me about msleep_interruptible()
> 
> To be honest, I think the arguments for using msleep() in general are
> far stronger. msleep_interruptible() was recently introduced to have the
> same effect except allow for a TASK_INTERRUPTIBLE based sleep. Since you
> are checking for a signal after returning from schedule_timeout(),
> msleep() isn't an option here ... What can be done here, actually, I
> just realized is to remove the signal_pending() check, as
> msleep_interruptible() does this as well.

Yes I think thats fine.

> There is another thing my patch didn't do, though - the
> 
> 	current->state = TASK_RUNNING;
> 
> call is unnecessary, because schedule_timeout() / msleep_interruptible()
> takes care of this.

Yes this seems fine.

> I have made these changes in the following patch -- please revert the
> previous cyclades patch and apply this one. Let me know if this is more
> satisfactory (it definitely simplifies the while-loop a bit).

Sure I can apply this. Its a good cleanup after all.

Thanks!

> Description: Uses msleep_interruptible() instead of schedule_timeout().
> This allows the removal of one conditional, as msleep_interruptible()
> should only return non-zero if signal_pending(current) is true.
> current->state also does not need to be set, as msleep_interruptible()
> (actually, schedule_timeout) is guaranteed to return in TASK_RUNNING.
> 
> Signed-off-by: Nishanth Aravamudan <nacc@us.ibm.com>
> 
> --- 2.6.9-rc2-vanilla/drivers/char/cyclades.c	2004-09-13 17:15:49.000000000 -0700
> +++ 2.6.9-rc2/drivers/char/cyclades.c	2004-10-01 13:55:19.000000000 -0700
> @@ -2717,20 +2717,16 @@ cy_wait_until_sent(struct tty_struct *tt
>  #ifdef CY_DEBUG_WAIT_UNTIL_SENT
>  	    printk("Not clean (jiff=%lu)...", jiffies);
>  #endif
> -	    current->state = TASK_INTERRUPTIBLE;
> -	    schedule_timeout(char_time);
> -	    if (signal_pending(current))
> +	    if (msleep_interruptible(jiffies_to_msecs(char_time)))
>  		break;
>  	    if (timeout && time_after(jiffies, orig_jiffies + timeout))
>  		break;
>  	}
> -	current->state = TASK_RUNNING;
>      } else {
>  	// Nothing to do!
>      }
>      /* Run one more char cycle */
> -    current->state = TASK_INTERRUPTIBLE;
> -    schedule_timeout(char_time * 5);
> +    msleep_interruptible(jiffies_to_msecs(char_time * 5));
>  #ifdef CY_DEBUG_WAIT_UNTIL_SENT
>      printk("Clean (jiff=%lu)...done\n", jiffies);
>  #endif
> @@ -2860,8 +2856,7 @@ cy_close(struct tty_struct *tty, struct 
>      if (info->blocked_open) {
>  	CY_UNLOCK(info, flags);
>          if (info->close_delay) {
> -            current->state = TASK_INTERRUPTIBLE;
> -            schedule_timeout(info->close_delay);
> +            msleep_interruptible(jiffies_to_msecs(info->close_delay));
>          }
>          wake_up_interruptible(&info->open_wait);
>  	CY_LOCK(info, flags);

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