Re: [RFC][PATCH] tty: remove the BKL in n_tty.c

[Frédéric Weisbecker]

2008/10/13 Frederic Weisbecker <fweisbec@xxxxxxxxx>:
>
> By using a bkl tracer, I saw that the opost function in n_tty.c, used when we
> write to the console, requests a lock_kernel(). And since I'm displaying the
> trace during the tracing, I see this lock very often.
>
> The opost and opost_block functions in drivers/char/n_tty.c use
> the bkl to protect the field tty->column.
> Actually this attribute seems to be only touched on these two functions and
> also we are in user context.
>
> A mutex seems to be sufficient to prevent from race conditions here.
> I made some tests by writing on a tty console whith several threads and it
> seems to be correct.
>
> I don't know where to post this patch. I'm not sure if the kill-the-bkl is
> still on the air on the -tip tree.
>
> Any comments?
>
> Signed-off-by: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> ---
> diff --git a/drivers/char/n_tty.c b/drivers/char/n_tty.c
> index 708c2b1..55c963f 100644
> --- a/drivers/char/n_tty.c
> +++ b/drivers/char/n_tty.c
> @@ -62,6 +62,13 @@
>  #define TTY_THRESHOLD_THROTTLE         128 /* now based on remaining room */
>  #define TTY_THRESHOLD_UNTHROTTLE       128
>
> +
> +/*
> + * This Mutex is used to replace a big kernel lock that protected
> + * the access to tty->column.
> + */
> +static DEFINE_MUTEX(tty_column_mutex);
> +
>  static inline unsigned char *alloc_buf(void)
>  {
>        gfp_t prio = in_interrupt() ? GFP_ATOMIC : GFP_KERNEL;
> @@ -264,18 +271,19 @@ static inline int is_continuation(unsigned char c, struct tty_struct *tty)
>  *     relevant in the world today. If you ever need them, add them here.
>  *
>  *     Called from both the receive and transmit sides and can be called
> - *     re-entrantly. Relies on lock_kernel() for tty->column state.
> + *     re-entrantly.
>  */
>
>  static int opost(unsigned char c, struct tty_struct *tty)
>  {
>        int     space, spaces;
> +       int     ret = 0;
>
>        space = tty_write_room(tty);
>        if (!space)
>                return -1;
>
> -       lock_kernel();
> +       mutex_lock(&tty_column_mutex);
>        if (O_OPOST(tty)) {
>                switch (c) {
>                case '\n':
> @@ -283,8 +291,8 @@ static int opost(unsigned char c, struct tty_struct *tty)
>                                tty->column = 0;
>                        if (O_ONLCR(tty)) {
>                                if (space < 2) {
> -                                       unlock_kernel();
> -                                       return -1;
> +                                       ret = -1;
> +                                       goto out;
>                                }
>                                tty_put_char(tty, '\r');
>                                tty->column = 0;
> @@ -293,8 +301,7 @@ static int opost(unsigned char c, struct tty_struct *tty)
>                        break;
>                case '\r':
>                        if (O_ONOCR(tty) && tty->column == 0) {
> -                               unlock_kernel();
> -                               return 0;
> +                               goto out;
>                        }
>                        if (O_OCRNL(tty)) {
>                                c = '\n';
> @@ -308,13 +315,12 @@ static int opost(unsigned char c, struct tty_struct *tty)
>                        spaces = 8 - (tty->column & 7);
>                        if (O_TABDLY(tty) == XTABS) {
>                                if (space < spaces) {
> -                                       unlock_kernel();
> -                                       return -1;
> +                                       ret = -1;
> +                                       goto out;
>                                }
>                                tty->column += spaces;
>                                tty->ops->write(tty, "        ", spaces);
> -                               unlock_kernel();
> -                               return 0;
> +                               goto out;
>                        }
>                        tty->column += spaces;
>                        break;
> @@ -331,8 +337,9 @@ static int opost(unsigned char c, struct tty_struct *tty)
>                }
>        }
>        tty_put_char(tty, c);
> -       unlock_kernel();
> -       return 0;
> +out:
> +       mutex_unlock(&tty_column_mutex);
> +       return ret;
>  }
>
>  /**
> @@ -346,8 +353,7 @@ static int opost(unsigned char c, struct tty_struct *tty)
>  *     the simple cases normally found and helps to generate blocks of
>  *     symbols for the console driver and thus improve performance.
>  *
> - *     Called from write_chan under the tty layer write lock. Relies
> - *     on lock_kernel for the tty->column state.
> + *     Called from write_chan under the tty layer write lock.
>  */
>
>  static ssize_t opost_block(struct tty_struct *tty,
> @@ -363,7 +369,7 @@ static ssize_t opost_block(struct tty_struct *tty,
>        if (nr > space)
>                nr = space;
>
> -       lock_kernel();
> +       mutex_lock(&tty_column_mutex);
>        for (i = 0, cp = buf; i < nr; i++, cp++) {
>                switch (*cp) {
>                case '\n':
> @@ -398,7 +404,8 @@ break_out:
>        if (tty->ops->flush_chars)
>                tty->ops->flush_chars(tty);
>        i = tty->ops->write(tty, buf, i);
> -       unlock_kernel();
> +
> +       mutex_unlock(&tty_column_mutex);
>        return i;
>  }
>
>
>

(Ading Alan in Cc)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/