Success: tty_io flush_to_ldisc() error message triggered
Subsystems (2)
Chuck Ebbert
Acked-byIn-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 *privateif (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 *privatetty_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
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
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
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.
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]
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
@@ -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
@@ -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
Re: [PATCH]: tty buffering limit
Quoted 3 lines · Alan Cox
Certain people seem to have assumed
tty->throttledwas 'advisory'. In the absence oftty->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.