Messages in this thread Patch in this message | | | From | Paul Mackerras <> | Subject | [PATCH] SMP problem in 2.2 n_tty code | Date | Wed, 23 Feb 2000 19:49:37 +1100 |
| |
I have tracked down why my ppp-over-pty-over-socket-over-ethernet test setup was clagging up on SMP machines with 2.2.15pre9. The problem is actually in drivers/char/n_tty.c.
The way the setup works is that I have a pty master/slave pair with the slave side set to PPP line discipline. The master side is in the normal (N_TTY) line discipline. There is a usermode process transferring characters between the master side and a socket.
The n_tty.c code has a receive buffer (tty->read_buf) which is used as a ring buffer addressed by tty->read_head and tty->read_tail, with tty->read_cnt being the number of characters in the buffer. What I was seeing was that these three were getting out of sync, so that for example at one stage read_head was 225, read_tail was 475 but read_cnt was 0.
The problem was basically the concurrency between the read_chan() function and the n_tty_receive_buf function. read_chan() attempts to control this by setting the TTY_DONT_FLIP bit, which prevents n_tty_receive_buf from being called from flush_to_ldisc in tty_io.c. But the pty_write function calls ldisc.receive_buf without checking the TTY_DONT_FLIP bit.
The patch below fixes the problem using a spinlock around all the sections in n_tty.c that update tty->read_{head,tail,cnt}.
The patch also fixes a potential problem in pty.c where it calls to->ldisc.receive_room() inside the MIN macro like this:
c = MIN(count, to->ldisc.receive_room(to));
Since receive_room can return a different value each time, we could end up with c > count.
With this patch, I can run my ppp-over-pty setup without any problems on an SMP machine.
Paul.
diff -urN official/include/linux/tty.h linux/include/linux/tty.h --- official/include/linux/tty.h Wed Jan 5 09:19:53 2000 +++ linux/include/linux/tty.h Wed Feb 23 13:45:08 2000 @@ -304,6 +304,7 @@ unsigned int canon_column; struct semaphore atomic_read; struct semaphore atomic_write; + spinlock_t read_lock; }; /* tty magic number */ diff -urN official/drivers/char/n_tty.c linux/drivers/char/n_tty.c --- official/drivers/char/n_tty.c Mon Feb 21 11:15:45 2000 +++ linux/drivers/char/n_tty.c Wed Feb 23 13:48:30 2000 @@ -62,8 +62,6 @@ #define TTY_THRESHOLD_THROTTLE 128 /* now based on remaining room */ #define TTY_THRESHOLD_UNTHROTTLE 128 -static spinlock_t __attribute((unused)) tty_put_lock = SPIN_LOCK_UNLOCKED; - static inline void put_tty_queue(unsigned char c, struct tty_struct *tty) { unsigned long flags; @@ -71,13 +69,13 @@ * The problem of stomping on the buffers ends here. * Why didn't anyone see this one comming? --AJK */ - spin_lock_irqsave(&tty_put_lock, flags); + spin_lock_irqsave(&tty->read_lock, flags); if (tty->read_cnt < N_TTY_BUF_SIZE) { tty->read_buf[tty->read_head] = c; tty->read_head = (tty->read_head + 1) & (N_TTY_BUF_SIZE-1); tty->read_cnt++; } - spin_unlock_irqrestore(&tty_put_lock, flags); + spin_unlock_irqrestore(&tty->read_lock, flags); } /* @@ -100,7 +98,11 @@ */ static void reset_buffer_flags(struct tty_struct *tty) { + unsigned long flags; + + spin_lock_irqsave(&tty->read_lock, flags); tty->read_head = tty->read_tail = tty->read_cnt = 0; + spin_unlock_irqrestore(&tty->read_lock, flags); tty->canon_head = tty->canon_data = tty->erasing = 0; memset(&tty->read_flags, 0, sizeof tty->read_flags); check_unthrottle(tty); @@ -128,14 +130,19 @@ */ ssize_t n_tty_chars_in_buffer(struct tty_struct *tty) { - if (tty->icanon) { - if (!tty->canon_data) return 0; + unsigned long flags; + ssize_t n = 0; - return (tty->canon_head > tty->read_tail) ? + spin_lock_irqsave(&tty->read_lock, flags); + if (!tty->icanon) { + n = tty->read_cnt; + } else if (tty->canon_data) { + n = (tty->canon_head > tty->read_tail) ? tty->canon_head - tty->read_tail : tty->canon_head + (N_TTY_BUF_SIZE - tty->read_tail); } - return tty->read_cnt; + spin_unlock_irqrestore(&tty->read_lock, flags); + return n; } /* @@ -297,6 +304,7 @@ { enum { ERASE, WERASE, KILL } kill_type; int head, seen_alnums; + unsigned long flags; if (tty->read_head == tty->canon_head) { /* opost('\a', tty); */ /* what do you think? */ @@ -308,15 +316,19 @@ kill_type = WERASE; else { if (!L_ECHO(tty)) { + spin_lock_irqsave(&tty->read_lock, flags); tty->read_cnt -= ((tty->read_head - tty->canon_head) & (N_TTY_BUF_SIZE - 1)); tty->read_head = tty->canon_head; + spin_unlock_irqrestore(&tty->read_lock, flags); return; } if (!L_ECHOK(tty) || !L_ECHOKE(tty) || !L_ECHOE(tty)) { + spin_lock_irqsave(&tty->read_lock, flags); tty->read_cnt -= ((tty->read_head - tty->canon_head) & (N_TTY_BUF_SIZE - 1)); tty->read_head = tty->canon_head; + spin_unlock_irqrestore(&tty->read_lock, flags); finish_erasing(tty); echo_char(KILL_CHAR(tty), tty); /* Add a newline if ECHOK is on and ECHOKE is off. */ @@ -338,8 +350,10 @@ else if (seen_alnums) break; } + spin_lock_irqsave(&tty->read_lock, flags); tty->read_head = head; tty->read_cnt--; + spin_unlock_irqrestore(&tty->read_lock, flags); if (L_ECHO(tty)) { if (L_ECHOPRT(tty)) { if (!tty->erasing) { @@ -672,11 +686,13 @@ char *f, flags = TTY_NORMAL; int i; char buf[64]; + unsigned long cpuflags; if (!tty->read_buf) return; if (tty->real_raw) { + spin_lock_irqsave(&tty->read_lock, cpuflags); i = MIN(count, MIN(N_TTY_BUF_SIZE - tty->read_cnt, N_TTY_BUF_SIZE - tty->read_head)); memcpy(tty->read_buf + tty->read_head, cp, i); @@ -690,6 +706,7 @@ memcpy(tty->read_buf + tty->read_head, cp, i); tty->read_head = (tty->read_head + i) & (N_TTY_BUF_SIZE-1); tty->read_cnt += i; + spin_unlock_irqrestore(&tty->read_lock, cpuflags); } else { for (i=count, p = cp, f = fp; i; i--, p++) { if (f) @@ -864,15 +881,20 @@ { int retval; ssize_t n; + unsigned long flags; retval = 0; + spin_lock_irqsave(&tty->read_lock, flags); n = MIN(*nr, MIN(tty->read_cnt, N_TTY_BUF_SIZE - tty->read_tail)); + spin_unlock_irqrestore(&tty->read_lock, flags); if (n) { mb(); retval = copy_to_user(*b, &tty->read_buf[tty->read_tail], n); n -= retval; + spin_lock_irqsave(&tty->read_lock, flags); tty->read_tail = (tty->read_tail + n) & (N_TTY_BUF_SIZE-1); tty->read_cnt -= n; + spin_unlock_irqrestore(&tty->read_lock, flags); *b += n; *nr -= n; } @@ -889,6 +911,7 @@ ssize_t retval = 0; ssize_t size; long timeout; + unsigned long flags; do_it_again: @@ -1007,9 +1030,11 @@ eol = test_and_clear_bit(tty->read_tail, &tty->read_flags); c = tty->read_buf[tty->read_tail]; + spin_lock_irqsave(&tty->read_lock, flags); tty->read_tail = ((tty->read_tail+1) & (N_TTY_BUF_SIZE-1)); tty->read_cnt--; + spin_unlock_irqrestore(&tty->read_lock, flags); if (!eol || (c != __DISABLED_CHAR)) { put_user(c, b++); diff -urN official/drivers/char/pty.c linux/drivers/char/pty.c --- official/drivers/char/pty.c Wed Feb 24 03:35:10 1999 +++ linux/drivers/char/pty.c Wed Feb 23 12:47:25 2000 @@ -168,7 +168,9 @@ } up(&tty->flip.pty_sem); } else { - c = MIN(count, to->ldisc.receive_room(to)); + c = to->ldisc.receive_room(to); + if (c > count) + c = count; to->ldisc.receive_buf(to, buf, 0, c); } diff -urN official/drivers/char/tty_io.c linux/drivers/char/tty_io.c --- official/drivers/char/tty_io.c Mon Feb 21 11:15:46 2000 +++ linux/drivers/char/tty_io.c Wed Feb 23 12:44:58 2000 @@ -1943,6 +1943,7 @@ tty->tq_hangup.data = tty; sema_init(&tty->atomic_read, 1); sema_init(&tty->atomic_write, 1); + spin_lock_init(&tty->read_lock); } /*
-- Paul Mackerras, Senior Open Source Researcher, Linuxcare, Inc. +61 2 6262 8990 tel, +61 2 6262 8991 fax paulus@linuxcare.com.au, http://www.linuxcare.com.au/ Linuxcare. Support for the revolution.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |