On Mon, 30 May 2022, cael wrote:
> Thanks, You are right, barrier is needed here. I changed the patch as follows:
> 1) WRITE_ONCE and READ_ONCE is used to access ldata->no_room since
> n_tty_kick_worker would be called in kworker and reader cpu;
> 2) smp_mb added in chars_in_buffer as this function will be called in
> reader and kworker, accessing commit_head and read_tail; and to make
> sure that read_tail is not read before setting no_room in
> n_tty_receive_buf_common;
> 3) smp_mb added in n_tty_read to make sure that no_room is not read
> before setting read_tail.
Please include proper changelog to all revised patch submissions, not
just list of changes you've made (and properly version the submissions
with [PATCH v2] etc. in the subject).
> ---
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index efc72104c840..3327687da0d3 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -201,8 +201,8 @@ static void n_tty_kick_worker(struct tty_struct *tty)
> struct n_tty_data *ldata = tty->disc_data;
>
> /* Did the input worker stop? Restart it */
> - if (unlikely(ldata->no_room)) {
> - ldata->no_room = 0;
> + if (unlikely(READ_ONCE(ldata->no_room))) {
> + WRITE_ONCE(ldata->no_room, 0);
> WARN_RATELIMIT(tty->port->itty == NULL,
> "scheduling with invalid itty\n");
> @@ -221,6 +221,7 @@ static ssize_t chars_in_buffer(struct tty_struct *tty)
> struct n_tty_data *ldata = tty->disc_data;
> ssize_t n = 0;
>
> + smp_mb();
You should add the reason in comment for any barriers you add.
> if (!ldata->icanon)
> n = ldata->commit_head - ldata->read_tail;
> else
> @@ -1632,7 +1633,7 @@ n_tty_receive_buf_common(struct tty_struct *tty,
> const unsigned char *cp,
> if (overflow && room < 0)
> ldata->read_head--;
> room = overflow;
> - ldata->no_room = flow && !room;
> + WRITE_ONCE(ldata->no_room, flow && !room);
> } else
> overflow = 0;
>
> @@ -1663,6 +1664,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);
> +
Instead of having the barrier in chars_in_buffer() perhaps it would be
more obvious what's going on here and also scope down to the cases where
the barrier might be needed in the first place if you'd do:
if (ldata->no_room) {
/* ... */
smp_mb();
if (!chars_in_buffer(tty))
n_tty_kick_worker(tty);
}
--
i.
> up_read(&tty->termios_rwsem);
>
> return rcvd;
> @@ -2180,8 +2184,10 @@ static ssize_t n_tty_read(struct tty_struct
> *tty, struct file *file,
> if (time)
> timeout = time;
> }
> - if (tail != ldata->read_tail)
> + if (tail != ldata->read_tail) {
> + smp_mb();
> n_tty_kick_worker(tty);
> + }
> up_read(&tty->termios_rwsem);
>
> remove_wait_queue(&tty->read_wait, &wait);
> --
> 2.27.0
>
> Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> 于2022年5月25日周三 19:21写道：
> >
> > On Wed, 25 May 2022, cael wrote:
> >
> > > >Now you switched to an entirely different case, not the one we were
> > > >talking about. ...There is no ldisc->no_room = true race in the case
> > > >you now described.
> > > So, I think we should back to the case ldata->no_room=true as
> > > ldata->no_room=false seems 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.
> > >
> > > As ldata->no_room=true is set before checking chars_in_buffer()
> >
> > Please take a brief look at Documentation/memory-barriers.txt and then
> > tell me if you still find this claim to be true.
> >
> > > if producer
> > > finds chars_in_buffer() > 0, then if reader is currently in n_tty_read,
> >
> > ...Then please do a similar analysis for ldata->read_tail. What guarantees
> > its update is seen by the producer cpu when the reader is already past the
> > point you think it still must be in?
> >
> > > when reader quits n_tty_read, n_tty_kick_worker will be called. If reader
> > > has already exited n_tty_read, which means that reader still has data to read,
> > > next time reader will call n_tty_kick_worker inside n_tty_read too.
> >
> > C-level analysis alone is not going to be very useful here given you're
> > dealing with a concurrency challenge here.
> >
> >
> > --
> > i.
> >
> >
>
