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

This is a discussion on [RFC][PATCH] tty: remove the BKL in n_tty.c - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 7 of 7

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

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


    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
    ---
    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;
    }


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

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

    2008/10/13 Frederic Weisbecker :
    >
    > 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
    > ---
    > 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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    > 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.


    There are patches pending for this already along with other n_tty
    ordering fixes, they will enter the ttydev tree once the current ttydev
    tree reaches Linus.

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

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

    2008/10/13 Alan Cox :
    > There are patches pending for this already along with other n_tty
    > ordering fixes, they will enter the ttydev tree once the current ttydev
    > tree reaches Linus.
    >
    > Alan
    >


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

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

    Frederic Weisbecker writes:
    >
    > +
    > +/*
    > + * This Mutex is used to replace a big kernel lock that protected
    > + * the access to tty->column.
    > + */
    > +static DEFINE_MUTEX(tty_column_mutex);


    I suppose Alan did it already, but the general rule when doing
    such changes is to lock data not code.

    -Andi

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

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

    > I suppose Alan did it already

    Credit goes to Joe Peterson not me. I looked at it several times and
    thought this will not be fun I'll do it later. In the meantime Joe
    debugged where others feared to tread and did all the work to drop the
    BKL out of those paths.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    2008/10/13 Andi Kleen :
    > Frederic Weisbecker writes:
    > I suppose Alan did it already, but the general rule when doing
    > such changes is to lock data not code.


    Ah ok. But there are so much references to tty->column on these functions
    so that I thought the best thing to do was to lock the whole path.

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

+ Reply to Thread