On Fri, Jan 29, 2021 at 11:57 PM Iris Johnson <i...@modwiz.com> wrote: > > Currently, the chardev backend code will prepare for IO polling to occur > by potentially adding or removing a watch of the backing channel for the > chardev. The chardev poll is added if the fd_can_read() function reports > more than 0 byte of buffer space, if a poll handler is already setup and > the bufer is now empty, the poll handler is removed. > > This causes a bug where the device buffer becomes ready, but the poll is > blocking on a sleep (potentially forever), because the buffer is small > and fills up immediately, while the backend channel has more data. This > leads to a stall condition or potentially a deadlock in the guest. > > The guest is looping, waiting for data to be reported as ready to read, > the host sees that the buffer is ready for reading and adds the poll, > the poll returns since data is available and data is made available to > the guest. Before the guest code is able to retrieve the data and clear > the full buffer, the poll code runs again, sees that the buffer is now > full, and removes the poll. At this point only a timeout from another > polled source, or another source having it's poll complete will result > in the loop running again to see that the buffer is now ready and to > add the poll again. > > We solve this issue by removing the logic that removes the poll, keeping > the existing logic to only create the poll once there's space for the > first read. > > Buglink: https://bugs.launchpad.net/qemu/+bug/1913341 > Signed-off-by: Iris Johnson <i...@modwiz.com> > --- > chardev/char-io.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/chardev/char-io.c b/chardev/char-io.c > index 8ced184160..fa9e222f78 100644 > --- a/chardev/char-io.c > +++ b/chardev/char-io.c > @@ -50,16 +50,14 @@ static gboolean io_watch_poll_prepare(GSource *source, > return FALSE; > } > > - if (now_active) { > + if (now_active && !was_active) { > iwp->src = qio_channel_create_watch( > iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL); > g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL); > g_source_add_child_source(source, iwp->src); > g_source_unref(iwp->src); > - } else { > - g_source_remove_child_source(source, iwp->src); > - iwp->src = NULL; > }
I don't think this works well enough: if the source isn't removed, but fd_can_read() returns 0, there is a potential busy loop on the next fd_read(). My understanding is that if data is read from the frontend, the loop will be re-entered and io_watch_poll_prepare will set the callback again. Could you provide a simple use-case or reproducer where we can evaluate how your patch improves the situation? thanks -- Marc-André Lureau
