char/cyclades: replace schedule_timeout() with msleep_interruptible()

PATCH New Marcelo Tosatti 1 participant 1 message Oct 4, 2004 Char and misc drivers

lore

Share & tools

lore

Marcelo Tosatti

author

2004-10-04 16:01 UTC #0 lore raw

PATCH 2/33 char/cyclades: replace schedule_timeout() with msleep_interruptible()

Andrew,

Nishanth's patch is a nice cleanup.

Please apply.

Germano, we should include this patch on top of
latest Linus's changes (which are on the mainline BK repository) to start the QA testing process.

----- Forwarded message from Nishanth Aravamudan <[email protected]> -----

From: Nishanth Aravamudan <[email protected]>
Date: Fri, 1 Oct 2004 14:09:01 -0700
To: Marcelo Tosatti <[email protected]>
Cc: [email protected], [email protected]
In-Reply-To: <[email protected]>
Subject: Re: [Kernel-janitors] [PATCH 2.6.9-rc2 2/33] char/cyclades: replace schedule_timeout() with msleep_interruptible()
X-Authentication-Warning: localhost.localdomain: nacc set sender to [email protected] using -f
X-Operating-System: Linux 2.6.8.1 (i686)
X-MIMETrack: Itemize by SMTP Server on USMail/Cyclades(Release 6.5.1|January 21, 2004) at
10/01/2004 14:11:25

Quoted 7 lines · Marcelo Tosatti

Quoted 6 lines · Nishanth Aravamudan
Quoted 4 lines · Marcelo Tosatti
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?

Quoted 8 lines

Quoted 5 lines
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

Quoted 6 lines

Quoted 4 lines
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...

Quoted 3 lines

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 <[email protected]>

drivers/char/cyclades.c +3 -8

@@ -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);