From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: cael <juanfengpy@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jiri Slaby <jirislaby@kernel.org>,
linux-serial <linux-serial@vger.kernel.org>
Subject: Re: tty: fix a possible hang on tty device
Date: Tue, 24 May 2022 14:40:15 +0300 (EEST) [thread overview]
Message-ID: <707c16ca-1610-68b1-fc96-4c5906f2c86@linux.intel.com> (raw)
In-Reply-To: <CAPmgiU+=uA9DrN13kAYb7VQ0xmfEA+xUduu+qEvp75qxFpZq7g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3377 bytes --]
On Tue, 24 May 2022, cael wrote:
> Thanks for the answer, yes, there exists a race between reader and kworker,
> but it's OK. Before checking chars_in_buffer in kworker,
> ldata->no_room is set true,
Nothing seems to guarantee this.
> if reader changes ldata->read_tail in n_tty_read when kworker checks this value
> which makes the check fail, then when reader reaches end of n_tty_read,
> n_tty_kick_worker will also be called. Besides, kworker and reader may
> call n_tty_kick_worker at the same time, this function only queues work
> on workqueue, so it's harmless.
I'm not worried about the case where both cpus call n_tty_kick_worker but
the case where producer cpu sees chars_in_buffer() > 0 and consumer cpu
!no_room.
--
i.
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> 于2022年5月24日周二 17:11写道:
> >
> > On Tue, 24 May 2022, cael wrote:
> >
> > > We have met a hang on pty device, the reader was blocking at
> > > epoll on master side, the writer was sleeping at wait_woken inside
> > > n_tty_write on slave side ,and the write buffer on tty_port was full, we
> >
> > Space after comma. It would be also useful to tone down usage of "we" in
> > the changelog.
> >
> > > found that the reader and writer would never be woken again and block
> > > forever.
> > >
> > > We thought the problem was caused as a race between reader and
> > > kworker as follows:
> > > n_tty_read(reader)| n_tty_receive_buf_common(kworker)
> > > |room = N_TTY_BUF_SIZE - (ldata->read_head - tail)
> > > |room <= 0
> > > copy_from_read_buf|
> > > n_tty_kick_worker |
> > > |ldata->no_room = true
> > >
> > > After writing to slave device, writer wakes up kworker to flush
> > > data on tty_port to reader, and the kworker finds that reader
> > > has no room to store data so room <= 0 is met. At this moment,
> > > reader consumes all the data on reader buffer and call
> > > n_tty_kick_worker to check ldata->no_room and finds that there
> > > is no need to call tty_buffer_restart_work to flush data to reader
> > > and reader quits reading. Then kworker sets ldata->no_room=true
> > > and quits too.
> > >
> > > If write buffer is not full, writer will wake kworker to flush data
> > > again after following writes, but if writer buffer is full and writer
> > > goes to sleep, kworker will never be woken again and tty device is
> > > blocked.
> > >
> > > We think this problem can be solved with a check for read buffer
> > > inside function n_tty_receive_buf_common, if read buffer is empty and
> > > ldata->no_room is true, this means that kworker has more data to flush
> > > to read buffer, so a call to n_tty_kick_worker is necessary.
> > >
> > > Signed-off-by: cael <juanfengpy@gmail.com>
> > > ---
> > > diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> > > index efc72104c840..36c7bc033c78 100644
> > > --- a/drivers/tty/n_tty.c
> > > +++ b/drivers/tty/n_tty.c
> > > @@ -1663,6 +1663,9 @@ n_tty_receive_buf_common(struct tty_struct *tty,
> > > const unsigned char *cp,
> > > } else
> > > n_tty_check_throttle(tty);
> > >
> > > + if (!chars_in_buffer(tty))
> > > + n_tty_kick_worker(tty);
> > > +
> >
> > chars_in_buffer() accesses ldata->read_tail in producer context so this
> > probably just moves the race there?
> >
> >
> > --
> > i.
> >
>
next prev parent reply other threads:[~2022-05-24 11:41 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-24 2:21 tty: fix a possible hang on tty device cael
2022-05-24 9:11 ` Ilpo Järvinen
2022-05-24 11:09 ` cael
2022-05-24 11:40 ` Ilpo Järvinen [this message]
2022-05-24 12:47 ` cael
2022-05-24 13:25 ` Ilpo Järvinen
2022-05-25 10:36 ` cael
2022-05-25 11:21 ` Ilpo Järvinen
2022-05-30 13:13 ` cael
2022-05-31 12:37 ` Ilpo Järvinen
2022-06-01 9:38 ` Greg KH
2022-06-01 13:39 ` cael
2022-06-01 14:47 ` Greg KH
2022-06-01 15:28 ` Ilpo Järvinen
2022-06-06 13:40 ` cael
2022-06-06 14:43 ` Greg KH
2022-06-11 6:50 ` cael
2022-06-11 7:32 ` Greg KH
2022-06-13 12:30 ` [PATCH v3] tty: fix hang on tty device with no_room set juanfengpy
2022-06-13 17:20 ` Greg KH
2022-06-15 3:45 ` [PATCH v4] " cael
2022-06-15 5:00 ` Greg KH
2022-06-15 7:57 ` Ilpo Järvinen
2022-06-15 9:29 ` Greg KH
2022-06-15 11:17 ` [PATCH v5] " cael
2022-06-15 11:29 ` Ilpo Järvinen
2022-06-15 13:33 ` caelli
2022-06-27 12:05 ` Greg KH
2022-06-27 13:53 ` [PATCH v6] " juanfengpy
2023-03-17 2:41 ` [PATCH v7] " juanfengpy
2023-03-17 6:32 ` Jiri Slaby
2023-03-17 7:25 ` [PATCH v8] " juanfengpy
2023-04-06 2:44 ` [PATCH v9] " juanfengpy
-- strict thread matches above, loose matches on Subject: below --
2022-05-07 9:11 tty: fix a possible hang on tty device cael
2022-05-17 10:22 ` Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=707c16ca-1610-68b1-fc96-4c5906f2c86@linux.intel.com \
--to=ilpo.jarvinen@linux.intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=jirislaby@kernel.org \
--cc=juanfengpy@gmail.com \
--cc=linux-serial@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).