Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1290472 > unrolled thread

[PATCH 5/6] n_tty: Fix stuck write wakeup

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 5/6] n_tty: Fix stuck write wakeup Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100
    Re: [PATCH 5/6] n_tty: Fix stuck write wakeup Johannes Stezenbach <js@sig21.net> - 2015-12-13 16:20 +0100
      Re: [PATCH 5/6] n_tty: Fix stuck write wakeup Peter Hurley <peter@hurleysoftware.com> - 2015-12-13 19:40 +0100
        Re: [PATCH 5/6] n_tty: Fix stuck write wakeup Johannes Stezenbach <js@sig21.net> - 2015-12-13 20:30 +0100

#1290472 — [PATCH 5/6] n_tty: Fix stuck write wakeup

If signal-driven i/o is disabled while write wakeup is pending (ie.,
n_tty_write() has set TTY_DO_WRITE_WAKEUP but then signal-driven i/o
is disabled), the TTY_DO_WRITE_WAKEUP bit will never be cleared and
will cause tty_wakeup() to always call n_tty_write_wakeup.

Unconditionally clear the write wakeup, and since kill_fasync()
already checks if the fasync ptr is null, call kill_fasync()
unconditionally as well.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/n_tty.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index e695f8f..5034da0 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -230,8 +230,8 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
 
 static void n_tty_write_wakeup(struct tty_struct *tty)
 {
-	if (tty->fasync && test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
-		kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
+	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
+	kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
 }
 
 static void n_tty_check_throttle(struct tty_struct *tty)
-- 
2.6.3

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

[toc] | [next] | [standalone]

#1290630

Hi Peter,

On Sat, Dec 12, 2015 at 02:16:38PM -0800, Peter Hurley wrote:
> If signal-driven i/o is disabled while write wakeup is pending (ie.,
> n_tty_write() has set TTY_DO_WRITE_WAKEUP but then signal-driven i/o
> is disabled), the TTY_DO_WRITE_WAKEUP bit will never be cleared and
> will cause tty_wakeup() to always call n_tty_write_wakeup.
> 
> Unconditionally clear the write wakeup, and since kill_fasync()
> already checks if the fasync ptr is null, call kill_fasync()
> unconditionally as well.
...
> @@ -230,8 +230,8 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
>  
>  static void n_tty_write_wakeup(struct tty_struct *tty)
>  {
> -	if (tty->fasync && test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
> -		kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
> +	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> +	kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
>  }

There is a related bug that I meant to send a patch, but I
never got around because the issue was found with proprietary
userspace and ancient kernel.  Maybe you could take care of it?
The patch might not apply cleanly after your recent changes
or might even be invalid now, please check.

Thanks,
Johannes


---
tty: n_tty: fix SIGIO for output

According to fcntl(2), "a SIGIO signal is sent whenever input
or output becomes possible on that file descriptor", i.e.
after the output buffer was full and now has space for new data.
But in fact SIGIO is sent after every write.

n_tty_write() should set TTY_DO_WRITE_WAKEUP only when
not all data could be written to the buffer.

Signed-off-by: Johannes Stezenbach <js@sig21.net>

--- drivers/char/n_tty.c.orig   2015-11-02 22:26:04.124227148 +0100
+++ drivers/char/n_tty.c        2015-11-02 22:26:10.644212115 +0100
@@ -1925,6 +1925,7 @@ static ssize_t n_tty_write(struct tty_st
        DECLARE_WAITQUEUE(wait, current);
        int c;
        ssize_t retval = 0;
+       size_t count = nr;

        /* Job control check -- must be done at start (POSIX.1 7.1.1.4). */
        if (L_TOSTOP(tty) && file->f_op->write != redirected_tty_write) {
@@ -1991,7 +1992,7 @@ static ssize_t n_tty_write(struct tty_st
 break_out:
        __set_current_state(TASK_RUNNING);
        remove_wait_queue(&tty->write_wait, &wait);
-       if (b - buf != nr && tty->fasync)
+       if (b - buf != count && tty->fasync)
                set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
        return (b - buf) ? b - buf : retval;
 }


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

[toc] | [prev] | [next] | [standalone]

#1290666

On 12/13/2015 07:18 AM, Johannes Stezenbach wrote:
> Hi Peter,
> 
> On Sat, Dec 12, 2015 at 02:16:38PM -0800, Peter Hurley wrote:
>> If signal-driven i/o is disabled while write wakeup is pending (ie.,
>> n_tty_write() has set TTY_DO_WRITE_WAKEUP but then signal-driven i/o
>> is disabled), the TTY_DO_WRITE_WAKEUP bit will never be cleared and
>> will cause tty_wakeup() to always call n_tty_write_wakeup.
>>
>> Unconditionally clear the write wakeup, and since kill_fasync()
>> already checks if the fasync ptr is null, call kill_fasync()
>> unconditionally as well.
> ...
>> @@ -230,8 +230,8 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
>>  
>>  static void n_tty_write_wakeup(struct tty_struct *tty)
>>  {
>> -	if (tty->fasync && test_and_clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags))
>> -		kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
>> +	clear_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>> +	kill_fasync(&tty->fasync, SIGIO, POLL_OUT);
>>  }
> 
> There is a related bug that I meant to send a patch, but I
> never got around because the issue was found with proprietary
> userspace and ancient kernel.  Maybe you could take care of it?
> The patch might not apply cleanly after your recent changes
> or might even be invalid now, please check.

Thanks for the patch, Johannes!

Yes, the patch below is still required to prevent excessive SIGIO
(and to prevent missed SIGIO when the amount actually copied just
happens to be exactly the amount left to be copied).

I made some comments in the patch; can you re-submit with those
changes and the patch title in the subject? Or I'd happy to re-work
it and send it to Greg if you'd prefer; just let me know.

Regards,
Peter Hurley

> ---
> tty: n_tty: fix SIGIO for output
> 
> According to fcntl(2), "a SIGIO signal is sent whenever input
> or output becomes possible on that file descriptor", i.e.
> after the output buffer was full and now has space for new data.
> But in fact SIGIO is sent after every write.
> 
> n_tty_write() should set TTY_DO_WRITE_WAKEUP only when
> not all data could be written to the buffer.
> 
> Signed-off-by: Johannes Stezenbach <js@sig21.net>
> 
> --- drivers/char/n_tty.c.orig   2015-11-02 22:26:04.124227148 +0100
> +++ drivers/char/n_tty.c        2015-11-02 22:26:10.644212115 +0100
> @@ -1925,6 +1925,7 @@ static ssize_t n_tty_write(struct tty_st
>         DECLARE_WAITQUEUE(wait, current);
>         int c;
>         ssize_t retval = 0;
> +       size_t count = nr;

'count' isn't required because after exiting the write loop, 'nr' will
be the remainder still to write so ...

> 
>         /* Job control check -- must be done at start (POSIX.1 7.1.1.4). */
>         if (L_TOSTOP(tty) && file->f_op->write != redirected_tty_write) {
> @@ -1991,7 +1992,7 @@ static ssize_t n_tty_write(struct tty_st
>  break_out:
>         __set_current_state(TASK_RUNNING);
>         remove_wait_queue(&tty->write_wait, &wait);
> -       if (b - buf != nr && tty->fasync)
> +       if (b - buf != count && tty->fasync)

... this can be

	if (nr && tty->fasync)
		set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);

>                 set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
>         return (b - buf) ? b - buf : retval;
>  }
> 
> 

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

[toc] | [prev] | [next] | [standalone]

#1290678

On Sun, Dec 13, 2015 at 10:38:02AM -0800, Peter Hurley wrote:
> On 12/13/2015 07:18 AM, Johannes Stezenbach wrote:
> > 
> > There is a related bug that I meant to send a patch, but I
> > never got around because the issue was found with proprietary
> > userspace and ancient kernel.  Maybe you could take care of it?
> > The patch might not apply cleanly after your recent changes
> > or might even be invalid now, please check.
> 
> Thanks for the patch, Johannes!
> 
> Yes, the patch below is still required to prevent excessive SIGIO
> (and to prevent missed SIGIO when the amount actually copied just
> happens to be exactly the amount left to be copied).
> 
> I made some comments in the patch; can you re-submit with those
> changes and the patch title in the subject? Or I'd happy to re-work
> it and send it to Greg if you'd prefer; just let me know.

Please rework it, currently I'm in lazy bum mode ;-)

> > @@ -1991,7 +1992,7 @@ static ssize_t n_tty_write(struct tty_st
> >  break_out:
> >         __set_current_state(TASK_RUNNING);
> >         remove_wait_queue(&tty->write_wait, &wait);
> > -       if (b - buf != nr && tty->fasync)
> > +       if (b - buf != count && tty->fasync)
> 
> ... this can be
> 
> 	if (nr && tty->fasync)
> 		set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);

Yeah, that's way better.

Thanks,
Johannes
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web