[PATCH 3/3] tty_io.c: don't use flush_scheduled_work()

[PATCH 3/3] tty_io.c: don't use flush_scheduled_work()

[Oleg Nesterov]

I don't know how to test this patch, the ack/nack from maintainer is wanted.

flush_scheduled_work() is evil and should be avoided. Change tty_set_ldisc()
and release_dev() to use cancel_delayed_work_sync/cancel_work_sync.

I am not sure we really need to call do_tty_hangup() when cancel_work_sync()
returns true, but this matches the current behaviour.

Also, some whitespace fixes.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- t/drivers/char/tty_io.c~	2007-07-01 18:00:10.000000000 +0400
+++ t/drivers/char/tty_io.c	2007-07-01 19:22:25.000000000 +0400
@@ -162,6 +162,8 @@ static void release_tty(struct tty_struc
 static void __proc_set_tty(struct task_struct *tsk, struct tty_struct *tty);
 static void proc_set_tty(struct task_struct *tsk, struct tty_struct *tty);
 
+static void do_tty_hangup(struct work_struct *work);
+
 /**
  *	alloc_tty_struct	-	allocate a tty object
  *
@@ -1038,12 +1040,10 @@ restart:
 	 *	we say so later on.
 	 */
 
-	work = cancel_delayed_work(&tty->buf.work);
-	/*
-	 * Wait for ->hangup_work and ->buf.work handlers to terminate
-	 */
-	 
-	flush_scheduled_work();
+	work = cancel_delayed_work_sync(&tty->buf.work);
+	if (cancel_work_sync(&tty->hangup_work))
+		do_tty_hangup(&tty->hangup_work);
+
 	/* Shutdown the current discipline. */
 	if (tty->ldisc.close)
 		(tty->ldisc.close)(tty);
@@ -1074,23 +1074,22 @@ restart:
 		}
 	}
 	/* At this point we hold a reference to the new ldisc and a
-	   a reference to the old ldisc. If we ended up flipping back
-	   to the existing ldisc we have two references to it */
-	
+	 * a reference to the old ldisc. If we ended up flipping back
+	 * to the existing ldisc we have two references to it
+	 */
 	if (tty->ldisc.num != o_ldisc.num && tty->driver->set_ldisc)
 		tty->driver->set_ldisc(tty);
-		
+
 	tty_ldisc_put(o_ldisc.num);
-	
+
 	/*
 	 *	Allow ldisc referencing to occur as soon as the driver
 	 *	ldisc callback completes.
 	 */
-	 
 	tty_ldisc_enable(tty);
 	if (o_tty)
 		tty_ldisc_enable(o_tty);
-	
+
 	/* Restart it in case no characters kick it off. Safe if
 	   already running */
 	if (work)
@@ -2457,7 +2456,7 @@ static void release_dev(struct file * fi
 	/* check whether both sides are closing ... */
 	if (!tty_closing || (o_tty && !o_tty_closing))
 		return;
-	
+
 #ifdef TTY_DEBUG_HANGUP
 	printk(KERN_DEBUG "freeing tty structure...");
 #endif
@@ -2467,15 +2466,11 @@ static void release_dev(struct file * fi
 	 * race with the set_ldisc code path.
 	 */
 	clear_bit(TTY_LDISC, &tty->flags);
-	cancel_delayed_work(&tty->buf.work);
+	cancel_delayed_work_sync(&tty->buf.work);
+	if (cancel_work_sync(&tty->hangup_work))
+		do_tty_hangup(&tty->hangup_work);
 
 	/*
-	 * Wait for ->hangup_work and ->buf.work handlers to terminate
-	 */
-	 
-	flush_scheduled_work();
-	
-	/*
 	 * Wait for any short term users (we know they are just driver
 	 * side waiters as the file is closing so user count on the file
 	 * side is zero.
@@ -2497,7 +2492,7 @@ static void release_dev(struct file * fi
 	if (tty->ldisc.close)
 		(tty->ldisc.close)(tty);
 	tty_ldisc_put(tty->ldisc.num);
-	
+
 	/*
 	 *	Switch the line discipline back
 	 */

Re: [PATCH 3/3] tty_io.c: don't use flush_scheduled_work()

[Alan Cox]

On Sun, 1 Jul 2007 19:37:49 +0400
Oleg Nesterov <oleg@tv-sign.ru> wrote:

> I don't know how to test this patch, the ack/nack from maintainer is wanted.
> 
> flush_scheduled_work() is evil and should be avoided. Change tty_set_ldisc()
> and release_dev() to use cancel_delayed_work_sync/cancel_work_sync.
> 
> I am not sure we really need to call do_tty_hangup() when cancel_work_sync()
> returns true, but this matches the current behaviour.
> 
> Also, some whitespace fixes.
> 
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

I'm sorry but the tty layer hangup code is not in a shape to do these
changes. Please leave it alone until the revoke() code in 2.6.2x-mm is
ready for mainstream then we will switch to that and all the mess goes
away.

Its just *too* fragile to touch otherwise, and we've had repeated
breakages from tiny changes in this area.

Alan

Re: [PATCH 3/3] tty_io.c: don't use flush_scheduled_work()

[Oleg Nesterov]

On 07/01, Alan Cox wrote:
>
> On Sun, 1 Jul 2007 19:37:49 +0400
> Oleg Nesterov <oleg@tv-sign.ru> wrote:
> 
> > I don't know how to test this patch, the ack/nack from maintainer is wanted.
> > 
> > flush_scheduled_work() is evil and should be avoided. Change tty_set_ldisc()
> > and release_dev() to use cancel_delayed_work_sync/cancel_work_sync.
> > 
> > I am not sure we really need to call do_tty_hangup() when cancel_work_sync()
> > returns true, but this matches the current behaviour.
> > 
> > Also, some whitespace fixes.
> > 
> > Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
> I'm sorry but the tty layer hangup code is not in a shape to do these
> changes. Please leave it alone until the revoke() code in 2.6.2x-mm is
> ready for mainstream then we will switch to that and all the mess goes
> away.
> 
> Its just *too* fragile to touch otherwise, and we've had repeated
> breakages from tiny changes in this area.

OK, thanks, please ignore this patch then. I did it just as example how
to avoid flush_workqueue() when we can't just kill the work.

Oleg.

Re: [PATCH 3/3] tty_io.c: don't use flush_scheduled_work()

[Dan Aloni]

On Sun, Jul 01, 2007 at 07:37:49PM +0400, Oleg Nesterov wrote:
> I don't know how to test this patch, the ack/nack from maintainer is wanted.
> 
> flush_scheduled_work() is evil and should be avoided. Change tty_set_ldisc()
> and release_dev() to use cancel_delayed_work_sync/cancel_work_sync.
> 
> I am not sure we really need to call do_tty_hangup() when cancel_work_sync()
> returns true, but this matches the current behaviour.

I also noticed this problem recently with 2.6.22, on a 2-CPU box where there 
was one SCHED_RR userspace process stuck in a busy loop. The box was completely 
responsive but had this annoyance where all tty closings were stuck in 
flush_scheduled_work(). It's especially noticable when you ssh to the machine
and then try to log out.

A temporary workaround was to give just the workqueue events/* threads a 
SCHED_FIFO static priority of 99, but I have kept that small patch to 
myself (figured it's just too nasty).

-- 
Dan Aloni
XIV LTD, http://www.xivstorage.com
da-x (at) monatomic.org, dan (at) xiv.co.il

Re: [PATCH 3/3] tty_io.c: don't use flush_scheduled_work()

[Oleg Nesterov]

On 08/16, Dan Aloni wrote:
>
> On Sun, Jul 01, 2007 at 07:37:49PM +0400, Oleg Nesterov wrote:
> > I don't know how to test this patch, the ack/nack from maintainer is wanted.
> > 
> > flush_scheduled_work() is evil and should be avoided. Change tty_set_ldisc()
> > and release_dev() to use cancel_delayed_work_sync/cancel_work_sync.
> > 
> > I am not sure we really need to call do_tty_hangup() when cancel_work_sync()
> > returns true, but this matches the current behaviour.
> 
> I also noticed this problem recently with 2.6.22, on a 2-CPU box where there 
> was one SCHED_RR userspace process stuck in a busy loop. The box was completely 
> responsive but had this annoyance where all tty closings were stuck in 
> flush_scheduled_work(). It's especially noticable when you ssh to the machine
> and then try to log out.

cancel_work_sync(work) can hang too if some SCHED_RR userspace process does not
relinquish CPU, but the probability is much lower (it should preempt work->func
of that work_struct).

see also http://marc.info/?l=linux-kernel&m=118115098120503.

Oleg.

Re: [PATCH 3/3] tty_io.c: don't use flush_scheduled_work()

[Jarek Poplawski]

On Thu, Aug 16, 2007 at 02:53:50PM +0300, Dan Aloni wrote:
> On Sun, Jul 01, 2007 at 07:37:49PM +0400, Oleg Nesterov wrote:
> > I don't know how to test this patch, the ack/nack from maintainer is wanted.
> > 
> > flush_scheduled_work() is evil and should be avoided. Change tty_set_ldisc()
> > and release_dev() to use cancel_delayed_work_sync/cancel_work_sync.
> > 
> > I am not sure we really need to call do_tty_hangup() when cancel_work_sync()
> > returns true, but this matches the current behaviour.
> 
> I also noticed this problem recently with 2.6.22, on a 2-CPU box where there 
> was one SCHED_RR userspace process stuck in a busy loop. The box was completely 

IMHO, it was rather a busy sleep.

> responsive but had this annoyance where all tty closings were stuck in 
> flush_scheduled_work(). It's especially noticable when you ssh to the machine
> and then try to log out.
> 
> A temporary workaround was to give just the workqueue events/* threads a 
> SCHED_FIFO static priority of 99, but I have kept that small patch to 
> myself (figured it's just too nasty).

It looks like there was something more than this one SCHED_RR:
probably some high priority task(s) could have preempted workqueue
thread, delaying run_workqueues. Then it should be an interesting test
for this new, 2.6.23 scheduler.

Regards,
Jarek P.

PS: sorry for so delayed responsing.