n_tty: Make N_TTY ldisc receive path lockless
authorPeter Hurley <peter@hurleysoftware.com>
Sat, 15 Jun 2013 13:14:26 +0000 (09:14 -0400)
committerGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Tue, 23 Jul 2013 23:43:01 +0000 (16:43 -0700)
n_tty has a single-producer/single-consumer input model;
use lockless publish instead.

Use termios_rwsem to exclude both consumer and producer while
changing or resetting buffer indices, eg., when flushing. Also,
claim exclusive termios_rwsem to safely retrieve the buffer
indices from a thread other than consumer or producer
(eg., TIOCINQ ioctl).

Note the read_tail is published _after_ clearing the newline
indicator in read_flags to avoid racing the producer.

Drop read_lock spinlock.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
drivers/tty/n_tty.c

index 1098dd73a4f7f9b51ed4070ef9669d42acdc6c23..c7f71cb8ee1ffed079158efe6ec2cac3b4c2eb28 100644 (file)
@@ -110,7 +110,6 @@ struct n_tty_data {
        struct mutex atomic_read_lock;
        struct mutex output_lock;
        struct mutex echo_lock;
-       raw_spinlock_t read_lock;
 };
 
 static inline size_t read_cnt(struct n_tty_data *ldata)
@@ -168,7 +167,10 @@ static int receive_room(struct tty_struct *tty)
  *
  *     Re-schedules the flip buffer work if space just became available.
  *
- *     Locks: Concurrent update is protected with read_lock
+ *     Caller holds exclusive termios_rwsem
+ *        or
+ *     n_tty_read()/consumer path:
+ *             holds non-exclusive termios_rwsem
  */
 
 static void n_tty_set_room(struct tty_struct *tty)
@@ -191,34 +193,27 @@ static void n_tty_set_room(struct tty_struct *tty)
        }
 }
 
-static void put_tty_queue_nolock(unsigned char c, struct n_tty_data *ldata)
-{
-       if (read_cnt(ldata) < N_TTY_BUF_SIZE) {
-               *read_buf_addr(ldata, ldata->read_head) = c;
-               ldata->read_head++;
-       }
-}
-
 /**
  *     put_tty_queue           -       add character to tty
  *     @c: character
  *     @ldata: n_tty data
  *
- *     Add a character to the tty read_buf queue. This is done under the
- *     read_lock to serialize character addition and also to protect us
- *     against parallel reads or flushes
+ *     Add a character to the tty read_buf queue.
+ *
+ *     n_tty_receive_buf()/producer path:
+ *             caller holds non-exclusive termios_rwsem
+ *             modifies read_head
+ *
+ *     read_head is only considered 'published' if canonical mode is
+ *     not active.
  */
 
 static void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
 {
-       unsigned long flags;
-       /*
-        *      The problem of stomping on the buffers ends here.
-        *      Why didn't anyone see this one coming? --AJK
-       */
-       raw_spin_lock_irqsave(&ldata->read_lock, flags);
-       put_tty_queue_nolock(c, ldata);
-       raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
+       if (read_cnt(ldata) < N_TTY_BUF_SIZE) {
+               *read_buf_addr(ldata, ldata->read_head) = c;
+               ldata->read_head++;
+       }
 }
 
 /**
@@ -228,16 +223,13 @@ static void put_tty_queue(unsigned char c, struct n_tty_data *ldata)
  *     Reset the read buffer counters and clear the flags.
  *     Called from n_tty_open() and n_tty_flush_buffer().
  *
- *     Locking: tty_read_lock for read fields.
+ *     Locking: caller holds exclusive termios_rwsem
+ *              (or locking is not required)
  */
 
 static void reset_buffer_flags(struct n_tty_data *ldata)
 {
-       unsigned long flags;
-
-       raw_spin_lock_irqsave(&ldata->read_lock, flags);
        ldata->read_head = ldata->canon_head = ldata->read_tail = 0;
-       raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
 
        mutex_lock(&ldata->echo_lock);
        ldata->echo_pos = ldata->echo_cnt = ldata->echo_overrun = 0;
@@ -267,47 +259,55 @@ static void n_tty_packet_mode_flush(struct tty_struct *tty)
  *     buffer flushed (eg at hangup) or when the N_TTY line discipline
  *     internally has to clean the pending queue (for example some signals).
  *
- *     Locking: ctrl_lock, read_lock.
+ *     Holds termios_rwsem to exclude producer/consumer while
+ *     buffer indices are reset.
+ *
+ *     Locking: ctrl_lock, exclusive termios_rwsem
  */
 
 static void n_tty_flush_buffer(struct tty_struct *tty)
 {
+       down_write(&tty->termios_rwsem);
        reset_buffer_flags(tty->disc_data);
        n_tty_set_room(tty);
 
        if (tty->link)
                n_tty_packet_mode_flush(tty);
+       up_write(&tty->termios_rwsem);
 }
 
-/**
- *     n_tty_chars_in_buffer   -       report available bytes
- *     @tty: tty device
- *
- *     Report the number of characters buffered to be delivered to user
- *     at this instant in time.
- *
- *     Locking: read_lock
- */
-
 static ssize_t chars_in_buffer(struct tty_struct *tty)
 {
        struct n_tty_data *ldata = tty->disc_data;
-       unsigned long flags;
        ssize_t n = 0;
 
-       raw_spin_lock_irqsave(&ldata->read_lock, flags);
        if (!ldata->icanon)
                n = read_cnt(ldata);
        else
                n = ldata->canon_head - ldata->read_tail;
-       raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
        return n;
 }
 
+/**
+ *     n_tty_chars_in_buffer   -       report available bytes
+ *     @tty: tty device
+ *
+ *     Report the number of characters buffered to be delivered to user
+ *     at this instant in time.
+ *
+ *     Locking: exclusive termios_rwsem
+ */
+
 static ssize_t n_tty_chars_in_buffer(struct tty_struct *tty)
 {
+       ssize_t n;
+
        WARN_ONCE(1, "%s is deprecated and scheduled for removal.", __func__);
-       return chars_in_buffer(tty);
+
+       down_write(&tty->termios_rwsem);
+       n = chars_in_buffer(tty);
+       up_write(&tty->termios_rwsem);
+       return n;
 }
 
 /**
@@ -915,7 +915,12 @@ static inline void finish_erasing(struct n_tty_data *ldata)
  *     present in the stream from the driver layer. Handles the complexities
  *     of UTF-8 multibyte symbols.
  *
- *     Locking: read_lock for tty buffers
+ *     n_tty_receive_buf()/producer path:
+ *             caller holds non-exclusive termios_rwsem
+ *             modifies read_head
+ *
+ *     Modifying the read_head is not considered a publish in this context
+ *     because canonical mode is active -- only canon_head publishes
  */
 
 static void eraser(unsigned char c, struct tty_struct *tty)
@@ -925,9 +930,7 @@ static void eraser(unsigned char c, struct tty_struct *tty)
        size_t head;
        size_t cnt;
        int seen_alnums;
-       unsigned long flags;
 
-       /* FIXME: locking needed ? */
        if (ldata->read_head == ldata->canon_head) {
                /* process_output('\a', tty); */ /* what do you think? */
                return;
@@ -938,15 +941,11 @@ static void eraser(unsigned char c, struct tty_struct *tty)
                kill_type = WERASE;
        else {
                if (!L_ECHO(tty)) {
-                       raw_spin_lock_irqsave(&ldata->read_lock, flags);
                        ldata->read_head = ldata->canon_head;
-                       raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
                        return;
                }
                if (!L_ECHOK(tty) || !L_ECHOKE(tty) || !L_ECHOE(tty)) {
-                       raw_spin_lock_irqsave(&ldata->read_lock, flags);
                        ldata->read_head = ldata->canon_head;
-                       raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
                        finish_erasing(ldata);
                        echo_char(KILL_CHAR(tty), tty);
                        /* Add a newline if ECHOK is on and ECHOKE is off. */
@@ -958,7 +957,6 @@ static void eraser(unsigned char c, struct tty_struct *tty)
        }
 
        seen_alnums = 0;
-       /* FIXME: Locking ?? */
        while (ldata->read_head != ldata->canon_head) {
                head = ldata->read_head;
 
@@ -980,9 +978,7 @@ static void eraser(unsigned char c, struct tty_struct *tty)
                                break;
                }
                cnt = ldata->read_head - head;
-               raw_spin_lock_irqsave(&ldata->read_lock, flags);
                ldata->read_head = head;
-               raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
                if (L_ECHO(tty)) {
                        if (L_ECHOPRT(tty)) {
                                if (!ldata->erasing) {
@@ -1071,7 +1067,11 @@ static inline void isig(int sig, struct tty_struct *tty)
  *     An RS232 break event has been hit in the incoming bitstream. This
  *     can cause a variety of events depending upon the termios settings.
  *
- *     Called from the receive_buf path so single threaded.
+ *     n_tty_receive_buf()/producer path:
+ *             caller holds non-exclusive termios_rwsem
+ *             publishes read_head via put_tty_queue()
+ *
+ *     Note: may get exclusive termios_rwsem if flushing input buffer
  */
 
 static inline void n_tty_receive_break(struct tty_struct *tty)
@@ -1083,8 +1083,11 @@ static inline void n_tty_receive_break(struct tty_struct *tty)
        if (I_BRKINT(tty)) {
                isig(SIGINT, tty);
                if (!L_NOFLSH(tty)) {
+                       /* flushing needs exclusive termios_rwsem */
+                       up_read(&tty->termios_rwsem);
                        n_tty_flush_buffer(tty);
                        tty_driver_flush_buffer(tty);
+                       down_read(&tty->termios_rwsem);
                }
                return;
        }
@@ -1131,7 +1134,11 @@ static inline void n_tty_receive_overrun(struct tty_struct *tty)
  *     @c: character
  *
  *     Process a parity error and queue the right data to indicate
- *     the error case if necessary. Locking as per n_tty_receive_buf.
+ *     the error case if necessary.
+ *
+ *     n_tty_receive_buf()/producer path:
+ *             caller holds non-exclusive termios_rwsem
+ *             publishes read_head via put_tty_queue()
  */
 static inline void n_tty_receive_parity_error(struct tty_struct *tty,
                                              unsigned char c)
@@ -1159,12 +1166,16 @@ static inline void n_tty_receive_parity_error(struct tty_struct *tty,
  *     Process an individual character of input received from the driver.
  *     This is serialized with respect to itself by the rules for the
  *     driver above.
+ *
+ *     n_tty_receive_buf()/producer path:
+ *             caller holds non-exclusive termios_rwsem
+ *             publishes canon_head if canonical mode is active
+ *             otherwise, publishes read_head via put_tty_queue()
  */
 
 static inline void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
 {
        struct n_tty_data *ldata = tty->disc_data;
-       unsigned long flags;
        int parmrk;
 
        if (ldata->raw) {
@@ -1253,8 +1264,11 @@ static inline void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
                if (c == SUSP_CHAR(tty)) {
 send_signal:
                        if (!L_NOFLSH(tty)) {
+                               /* flushing needs exclusive termios_rwsem */
+                               up_read(&tty->termios_rwsem);
                                n_tty_flush_buffer(tty);
                                tty_driver_flush_buffer(tty);
+                               down_read(&tty->termios_rwsem);
                        }
                        if (I_IXON(tty))
                                start_tty(tty);
@@ -1355,11 +1369,9 @@ send_signal:
                                put_tty_queue(c, ldata);
 
 handle_newline:
-                       raw_spin_lock_irqsave(&ldata->read_lock, flags);
                        set_bit(ldata->read_head & (N_TTY_BUF_SIZE - 1), ldata->read_flags);
-                       put_tty_queue_nolock(c, ldata);
+                       put_tty_queue(c, ldata);
                        ldata->canon_head = ldata->read_head;
-                       raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
                        kill_fasync(&tty->fasync, SIGIO, POLL_IN);
                        if (waitqueue_active(&tty->read_wait))
                                wake_up_interruptible(&tty->read_wait);
@@ -1420,6 +1432,10 @@ static void n_tty_write_wakeup(struct tty_struct *tty)
  *     been received. This function must be called from soft contexts
  *     not from interrupt context. The driver is responsible for making
  *     calls one at a time and in order (or using flush_to_ldisc)
+ *
+ *     n_tty_receive_buf()/producer path:
+ *             claims non-exclusive termios_rwsem
+ *             publishes read_head and canon_head
  */
 
 static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
@@ -1430,10 +1446,8 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
        char *f, flags = TTY_NORMAL;
        int     i;
        char    buf[64];
-       unsigned long cpuflags;
 
        if (ldata->real_raw) {
-               raw_spin_lock_irqsave(&ldata->read_lock, cpuflags);
                i = min(N_TTY_BUF_SIZE - read_cnt(ldata),
                        N_TTY_BUF_SIZE - (ldata->read_head & (N_TTY_BUF_SIZE - 1)));
                i = min(count, i);
@@ -1447,7 +1461,6 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
                i = min(count, i);
                memcpy(read_buf_addr(ldata, ldata->read_head), cp, i);
                ldata->read_head += i;
-               raw_spin_unlock_irqrestore(&ldata->read_lock, cpuflags);
        } else {
                for (i = count, p = cp, f = fp; i; i--, p++) {
                        if (f)
@@ -1677,7 +1690,6 @@ static int n_tty_open(struct tty_struct *tty)
        mutex_init(&ldata->atomic_read_lock);
        mutex_init(&ldata->output_lock);
        mutex_init(&ldata->echo_lock);
-       raw_spin_lock_init(&ldata->read_lock);
 
        /* These are ugly. Currently a malloc failure here can panic */
        ldata->read_buf = kzalloc(N_TTY_BUF_SIZE, GFP_KERNEL);
@@ -1733,6 +1745,9 @@ static inline int input_available_p(struct tty_struct *tty, int amt)
  *
  *     Called under the ldata->atomic_read_lock sem
  *
+ *     n_tty_read()/consumer path:
+ *             caller holds non-exclusive termios_rwsem
+ *             read_tail published
  */
 
 static int copy_from_read_buf(struct tty_struct *tty,
@@ -1743,27 +1758,22 @@ static int copy_from_read_buf(struct tty_struct *tty,
        struct n_tty_data *ldata = tty->disc_data;
        int retval;
        size_t n;
-       unsigned long flags;
        bool is_eof;
        size_t tail = ldata->read_tail & (N_TTY_BUF_SIZE - 1);
 
        retval = 0;
-       raw_spin_lock_irqsave(&ldata->read_lock, flags);
        n = min(read_cnt(ldata), N_TTY_BUF_SIZE - tail);
        n = min(*nr, n);
-       raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
        if (n) {
                retval = copy_to_user(*b, read_buf_addr(ldata, tail), n);
                n -= retval;
                is_eof = n == 1 && read_buf(ldata, tail) == EOF_CHAR(tty);
                tty_audit_add_data(tty, read_buf_addr(ldata, tail), n,
                                ldata->icanon);
-               raw_spin_lock_irqsave(&ldata->read_lock, flags);
                ldata->read_tail += n;
                /* Turn single EOF into zero-length read */
                if (L_EXTPROC(tty) && ldata->icanon && is_eof && !read_cnt(ldata))
                        n = 0;
-               raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
                *b += n;
                *nr -= n;
        }
@@ -1781,6 +1791,10 @@ static int copy_from_read_buf(struct tty_struct *tty,
  *     character into the user-space buffer.
  *
  *     Called under the atomic_read_lock mutex
+ *
+ *     n_tty_read()/consumer path:
+ *             caller holds non-exclusive termios_rwsem
+ *             read_tail published
  */
 
 static int canon_copy_from_read_buf(struct tty_struct *tty,
@@ -1788,21 +1802,15 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
                                    size_t *nr)
 {
        struct n_tty_data *ldata = tty->disc_data;
-       unsigned long flags;
        size_t n, size, more, c;
        size_t eol;
        size_t tail;
        int ret, found = 0;
 
        /* N.B. avoid overrun if nr == 0 */
-
-       raw_spin_lock_irqsave(&ldata->read_lock, flags);
-
        n = min(*nr, read_cnt(ldata));
-       if (!n) {
-               raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
+       if (!n)
                return 0;
-       }
 
        tail = ldata->read_tail & (N_TTY_BUF_SIZE - 1);
        size = min_t(size_t, tail + n, N_TTY_BUF_SIZE);
@@ -1830,8 +1838,6 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
        n_tty_trace("%s: eol:%zu found:%d n:%zu c:%zu size:%zu more:%zu\n",
                    __func__, eol, found, n, c, size, more);
 
-       raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
-
        if (n > size) {
                ret = copy_to_user(*b, read_buf_addr(ldata, tail), size);
                if (ret)
@@ -1845,11 +1851,10 @@ static int canon_copy_from_read_buf(struct tty_struct *tty,
        *b += n;
        *nr -= n;
 
-       raw_spin_lock_irqsave(&ldata->read_lock, flags);
-       ldata->read_tail += c;
        if (found)
-               __clear_bit(eol, ldata->read_flags);
-       raw_spin_unlock_irqrestore(&ldata->read_lock, flags);
+               clear_bit(eol, ldata->read_flags);
+       smp_mb__after_clear_bit();
+       ldata->read_tail += c;
 
        if (found)
                tty_audit_push(tty);
@@ -1913,6 +1918,10 @@ static int job_control(struct tty_struct *tty, struct file *file)
  *     a hangup. Always called in user context, may sleep.
  *
  *     This code must be sure never to sleep through a hangup.
+ *
+ *     n_tty_read()/consumer path:
+ *             claims non-exclusive termios_rwsem
+ *             publishes read_tail
  */
 
 static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
@@ -2279,10 +2288,12 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file,
        case TIOCOUTQ:
                return put_user(tty_chars_in_buffer(tty), (int __user *) arg);
        case TIOCINQ:
-               /* FIXME: Locking */
-               retval = read_cnt(ldata);
+               down_write(&tty->termios_rwsem);
                if (L_ICANON(tty))
                        retval = inq_canon(ldata);
+               else
+                       retval = read_cnt(ldata);
+               up_write(&tty->termios_rwsem);
                return put_user(retval, (unsigned int __user *) arg);
        default:
                return n_tty_ioctl_helper(tty, file, cmd, arg);