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.
---
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();
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);
+
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.
>
>
