Skip to contentSkip to conversation

Success: tty_io flush_to_ldisc() error message triggered

PATCH In review Chuck Ebbert 4 participants 7 messages Jul 22, 2006 – Jul 28, 2006 1 message not in the archive

Subsystems (2)
lore
Share & tools
lore

Chuck Ebbert

author Acked-by
#0 lore raw

In-Reply-To: <[email protected]>

Quoted 6 lines · Paul Fulghum

That confirms my thoughts on what went wrong:
multiple copies of the queued work (flush_to_ldisc) running in parallel and corrupting the free buffer list.

A cleaner fix for this is already
in the 2.6.18 rc series.

The cleaner fix looks more intrusive, though.

Is this simpler change (what I'm running but without the warning messages) the preferred fix for -stable?

From: Paul Fulghum <[email protected]>

Serialize flush_to_ldisc() per-device. Fixes free list corruption that causes lockup on SMP systems.

include/linux/tty.h +1 -0
@@ -266,6 +266,7 @@ struct tty_struct {
 #define TTY_PTY_LOCK 		16	/* pty private */
 #define TTY_NO_WRITE_SPLIT 	17	/* Preserve write boundaries to driver */
 #define TTY_HUPPED 		18	/* Post driver->hangup() */
+#define TTY_FLUSHING 		19	/* Flushing tty buffers to line discipline */
 
 #define TTY_WRITE_FLUSH(tty) tty_write_flush((tty))
 
drivers/char/tty_io.c +3 -4
@@ -2780,10 +2780,8 @@ static void flush_to_ldisc(void *private
 	if (disc == NULL)	/*  !TTY_LDISC */
 		return;
 
-	if (test_bit(TTY_DONT_FLIP, &tty->flags)) {
-		/*
-		 * Do it after the next timer tick:
-		 */
+	if (test_bit(TTY_DONT_FLIP, &tty->flags) ||
+	    test_and_set_bit(TTY_FLUSHING, &tty->flags)) {
 		schedule_delayed_work(&tty->buf.work, 1);
 		goto out;
 	}
@@ -2805,6 +2803,7 @@ static void flush_to_ldisc(void *private
 		tty_buffer_free(tty, tbuf);
 	}
 	spin_unlock_irqrestore(&tty->buf.lock, flags);
+	clear_bit(TTY_FLUSHING, &tty->flags);
 out:
 	tty_ldisc_deref(disc);
 }
Signature · 1 line
Chuck
#1 lore raw
Quoted 4 lines · Chuck Ebbert

The cleaner fix looks more intrusive, though.

Is this simpler change (what I'm running but without the warning messages) the preferred fix for -stable?

It fixes the problem.

The ugly thing about that patch is adding another TTY_XXX macro that is not really necessary. I created it as a quick and dirty way of testing my hypothesis about the problem.

I don't see a problem using it as a simple (albeit naive) approach to fix stable.

--
Paul Fulghum
Microgate Systems, Ltd

#2 lore raw
Quoted 7 lines · Paul Fulghum
Quoted 4 lines · Chuck Ebbert

The cleaner fix looks more intrusive, though.

Is this simpler change (what I'm running but without the warning messages) the preferred fix for -stable?

It fixes the problem.

So do you feel this patch should be added to the -stable kernel tree?

thanks,

greg k-h

#3 lore raw
Quoted 6 lines · Greg KH
Quoted 4 lines · Paul Fulghum
Quoted 2 lines · Chuck Ebbert

The cleaner fix looks more intrusive, though.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Is this simpler change (what I'm running but without the warning messages) the preferred fix for -stable?

It fixes the problem.

So do you feel this patch should be added to the -stable kernel tree?

No. Now that I think about it, adding that extra macro is just wrong even if temporary.

The real fix is equally simple, but in 2.6.18-rc it is intertwined with other more intrusive changes.

Let me make a new separate patch that does things the right way, which is simply removing the list head while processing the list so two instances to not trip over each other. I would have done so earlier, but I've been insanely busy with multiple work related deadlines (lame excuse I know).

I should post something tomorrow afternoon.

Signature · 2 lines
Paul Fulghum
Microgate Systems, Ltd.
#4 lore raw
Quoted 21 lines · Paul Fulghum
Quoted 6 lines · Greg KH
Quoted 4 lines · Paul Fulghum
Quoted 2 lines · Chuck Ebbert

The cleaner fix looks more intrusive, though.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Is this simpler change (what I'm running but without the warning messages) the preferred fix for -stable?

It fixes the problem.

So do you feel this patch should be added to the -stable kernel tree?

No. Now that I think about it, adding that extra macro is just wrong even if temporary.

The real fix is equally simple, but in 2.6.18-rc it is intertwined with other more intrusive changes.

Quoted 8 lines

Let me make a new separate patch that does things the right way, which is simply removing the list head while processing the list so two instances to not trip over each other. I would have done so earlier, but I've been insanely busy with multiple work related deadlines (lame excuse I know).

I should post something tomorrow afternoon.

Ok, we can wait, I'd rather have the proper fix instead of the band-aid.

Just send it to [email protected] when you have something that you feel comfortable with.

thanks,

greg k-h

A message that is not in the archive (sent off-list or not yet received): [email protected]

#6 lore raw

PATCH 1/1 : tty buffering limit

Certain people seem to have assumed tty->throttled was 'advisory'. In the absence of tty->author->throttle(), it seems we should keep a simple limit of our own to avoid problems when this occurs.

drivers/char/tty_io.c +14 -8
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.18-rc2-mm1/drivers/char/tty_io.c linux-2.6.18-rc2-mm1/drivers/char/tty_io.c
@@ -247,6 +247,7 @@
 		kfree(thead);
 	}
 	tty->buf.tail = NULL;
+	tty->buf.memory_used = 0;
 }
 
 static void tty_buffer_init(struct tty_struct *tty)
@@ -255,11 +256,16 @@
 	tty->buf.head = NULL;
 	tty->buf.tail = NULL;
 	tty->buf.free = NULL;
+	tty->buf.memory_used = 0;
 }
 
-static struct tty_buffer *tty_buffer_alloc(size_t size)
+static struct tty_buffer *tty_buffer_alloc(struct tty_struct *tty, size_t size)
 {
-	struct tty_buffer *p = kmalloc(sizeof(struct tty_buffer) + 2 * size, GFP_ATOMIC);
+	struct tty_buffer *p;
+
+	if (tty->buf.memory_used + size > 65536)
+		return NULL;
+	p = kmalloc(sizeof(struct tty_buffer) + 2 * size, GFP_ATOMIC);
 	if(p == NULL)
 		return NULL;
 	p->used = 0;
@@ -269,7 +275,7 @@
 	p->read = 0;
 	p->char_buf_ptr = (char *)(p->data);
 	p->flag_buf_ptr = (unsigned char *)p->char_buf_ptr + size;
-/* 	printk("Flip create %p\n", p); */
+	tty->buf.memory_used += size;
 	return p;
 }
 
@@ -279,7 +285,9 @@
 static void tty_buffer_free(struct tty_struct *tty, struct tty_buffer *b)
 {
 	/* Dumb strategy for now - should keep some stats */
-/* 	printk("Flip dispose %p\n", b); */
+	tty->buf.memory_used -= b->size;
+	WARN_ON(tty->buf.memory_used < 0);
+
 	if(b->size >= 512)
 		kfree(b);
 	else {
@@ -299,16 +307,14 @@
 			t->used = 0;
 			t->commit = 0;
 			t->read = 0;
-			/* DEBUG ONLY */
-			memset(t->data, '*', size);
-/* 			printk("Flip recycle %p\n", t); */
+			tty->buf.memory_used += t->size;
 			return t;
 		}
 		tbh = &((*tbh)->next);
 	}
 	/* Round the buffer size out */
 	size = (size + 0xFF) & ~ 0xFF;
-	return tty_buffer_alloc(size);
+	return tty_buffer_alloc(tty, size);
 	/* Should possibly check if this fails for the largest buffer we
 	   have queued and recycle that ? */
 }
include/linux/tty.h +1 -0
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.18-rc2-mm1/include/linux/tty.h linux-2.6.18-rc2-mm1/include/linux/tty.h
@@ -59,6 +59,7 @@
 	struct tty_buffer *head;	/* Queue head */
 	struct tty_buffer *tail;	/* Active buffer */
 	struct tty_buffer *free;	/* Free queue head */
+	int memory_used;		/* Buffer space used excluding free queue */
 };
 /*
  * The pty uses char_buf and flag_buf as a contiguous buffer
#7 lore raw

Re: [PATCH]: tty buffering limit

Quoted 3 lines · Alan Cox

Certain people seem to have assumed tty->throttled was 'advisory'. In the absence of tty->author->throttle(), it seems we should keep a simple limit of our own to avoid problems when this occurs.

Looks reasonable as a general failsafe. There may be other pathological cases it protects against.

I'll start banging on it.

Signature · 2 lines
Paul Fulghum
Microgate Systems, Ltd.