[Qemu-devel] A question about this commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2

[Qemu-devel] A question about this commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2

[Gaohaifeng (A)]

Hi Daniel & Paolo,

Commit 9894dc0c "char: convert from GIOChannel to QIOChannel", about

the below code segment:

-static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
+static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
{
     CharDriverState *chr = opaque;
     TCPCharDriver *s = chr->opaque;
@@ -2938,9 +2801,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     if (len > s->max_size)
         len = s->max_size;
     size = tcp_chr_recv(chr, (void *)buf, len);
-    if (size == 0 ||
-        (size < 0 &&
-         socket_error() != EAGAIN && socket_error() != EWOULDBLOCK)) {
+    if (size == 0 || size == -1) {
         /* connection closed */
         tcp_chr_disconnect(chr);
     } else if (size > 0) {

The old version will not call tcp_chr_disconnect when error is not EAGAIN.
The new version will call tcp_chr_disconnect when size==-1. From the tcp_chr_recv function we see EAGIN will return -1, so EAGIN will call tcp_chr_disconnect.

We meet an issue when Guest VM use DPDK(1.6.0) l2fwd, it may exit since link status is not up. The link is down because tcp_chr_disconnect will set it.

Can you explain it why EAGIN here changes the behavior ?

Thanks,
Haifeng Gao

Re: [Qemu-devel] A question about this commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2

[Daniel P. Berrange]

On Tue, Aug 23, 2016 at 08:57:44AM +0000, Gaohaifeng (A) wrote:
> Hi Daniel & Paolo,
> 
> Commit 9894dc0c "char: convert from GIOChannel to QIOChannel", about
> 
> the below code segment:
> 
> -static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
> +static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
> {
>      CharDriverState *chr = opaque;
>      TCPCharDriver *s = chr->opaque;
> @@ -2938,9 +2801,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
>      if (len > s->max_size)
>          len = s->max_size;
>      size = tcp_chr_recv(chr, (void *)buf, len);
> -    if (size == 0 ||
> -        (size < 0 &&
> -         socket_error() != EAGAIN && socket_error() != EWOULDBLOCK)) {
> +    if (size == 0 || size == -1) {
>          /* connection closed */
>          tcp_chr_disconnect(chr);
>      } else if (size > 0) {
> 
> The old version will not call tcp_chr_disconnect when error is not
> EAGAIN.
> The new version will call tcp_chr_disconnect when size==-1. From the
> tcp_chr_recv function we see EAGIN will return -1, so EAGIN will call
> tcp_chr_disconnect.
> 
> We meet an issue when Guest VM use DPDK(1.6.0) l2fwd, it may exit since
> link status is not up. The link is down because tcp_chr_disconnect will
> set it.
> 
> Can you explain it why EAGIN here changes the behavior ?

tcp_chr_read is used in a call to io_add_watch_poll() which sets up an
event loop watch so that tcp_chr_read is called when there is incoming
data to be read.  IOW, when tcp_chr_read is invoked, I would always
expect that at least 1 byte is available, and so EAGAIN should not
occurr.

So I'm puzzled why you would get EAGAIN at all, unless there is some
kind of race with other code also consuming bytes from the same
socket.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] A question about this commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2

[Gaohaifeng (A)]

On Tue, Aug 23, 2016 at 08:57:44AM +0000, Gaohaifeng (A) wrote:
> Hi Daniel & Paolo,
> > 
> > Commit 9894dc0c "char: convert from GIOChannel to QIOChannel", about
> > 
> > the below code segment:
> > 
> > -static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, 
> > void *opaque)
> > +static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, 
> > +void *opaque)
> > {
> >      CharDriverState *chr = opaque;
> >      TCPCharDriver *s = chr->opaque;
> > @@ -2938,9 +2801,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
> >      if (len > s->max_size)
> >          len = s->max_size;
> >      size = tcp_chr_recv(chr, (void *)buf, len);
> > -    if (size == 0 ||
> > -        (size < 0 &&
> > -         socket_error() != EAGAIN && socket_error() != EWOULDBLOCK)) {
> > +    if (size == 0 || size == -1) {
> >          /* connection closed */
> >          tcp_chr_disconnect(chr);
> >      } else if (size > 0) {
> > 
> > The old version will not call tcp_chr_disconnect when error is not 
> > EAGAIN.
> > The new version will call tcp_chr_disconnect when size==-1. From the 
> > tcp_chr_recv function we see EAGIN will return -1, so EAGIN will call 
> > tcp_chr_disconnect.
> > 
> > We meet an issue when Guest VM use DPDK(1.6.0) l2fwd, it may exit 
> > since link status is not up. The link is down because 
> > tcp_chr_disconnect will set it.
> > 
> > Can you explain it why EAGIN here changes the behavior ?

> tcp_chr_read is used in a call to io_add_watch_poll() which sets up an event loop watch so that tcp_chr_read is called when there is incoming data to be read.  IOW, when tcp_chr_read is invoked, I would always expect that at least 1 byte is available, and so EAGAIN should not occurr.
> 
> So I'm puzzled why you would get EAGAIN at all, unless there is some kind of race with other code also consuming bytes from the same socket.

It could easily reproduce this when repeating 'rmmod virtio_net;modprobe virtio_net'.
The call stack:
#0  tcp_chr_disconnect (chr=0x7f09d5b2d540) at qemu-char.c:2867
#1  0x00007f09d43b2106 in tcp_chr_read (chan=0x7f09d5b2df30, cond=G_IO_IN, opaque=0x7f09d5b2d540) at qemu-char.c:2917
#2  0x00007f09d46364fa in qio_channel_fd_source_dispatch (source=0x7f093e603b30, callback=0x7f09d43b1fcb <tcp_chr_read>, user_data=0x7f09d5b2d540) at io/channel-watch.c:84
#3  0x00007f09d2e9999a in g_main_context_dispatch () from /lib64/libglib-2.0.so.0
#4  0x00007f09d45bfc96 in glib_pollfds_poll () at main-loop.c:213
#5  0x00007f09d45bfd73 in os_host_main_loop_wait (timeout=991853) at main-loop.c:258
#6  0x00007f09d45bfe23 in main_loop_wait (nonblocking=0) at main-loop.c:506
#7  0x00007f09d43bbdd2 in main_loop () at vl.c:2119
#8  0x00007f09d43c380e in main (argc=56, argv=0x7ffe7ad669a8, envp=0x7ffe7ad66b70) at vl.c:4896
(gdb) f 1
#1  0x00007f09d43b2106 in tcp_chr_read (chan=0x7f09d5b2df30, cond=G_IO_IN, opaque=0x7f09d5b2d540) at qemu-char.c:2917
2917	        tcp_chr_disconnect(chr);
(gdb) p errno
$1 = 11

EAGAIN = 11

> 
> Regards,
> Daniel

Re: [Qemu-devel] A question about this commit 9894dc0cdcc397ee5b26370bc53da6d360a363c2

[Daniel P. Berrange]

On Thu, Aug 25, 2016 at 01:21:47PM +0000, Gaohaifeng (A) wrote:
> On Tue, Aug 23, 2016 at 08:57:44AM +0000, Gaohaifeng (A) wrote:
> > Hi Daniel & Paolo,
> > > 
> > > Commit 9894dc0c "char: convert from GIOChannel to QIOChannel", about
> > > 
> > > the below code segment:
> > > 
> > > -static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, 
> > > void *opaque)
> > > +static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, 
> > > +void *opaque)
> > > {
> > >      CharDriverState *chr = opaque;
> > >      TCPCharDriver *s = chr->opaque;
> > > @@ -2938,9 +2801,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
> > >      if (len > s->max_size)
> > >          len = s->max_size;
> > >      size = tcp_chr_recv(chr, (void *)buf, len);
> > > -    if (size == 0 ||
> > > -        (size < 0 &&
> > > -         socket_error() != EAGAIN && socket_error() != EWOULDBLOCK)) {
> > > +    if (size == 0 || size == -1) {
> > >          /* connection closed */
> > >          tcp_chr_disconnect(chr);
> > >      } else if (size > 0) {
> > > 
> > > The old version will not call tcp_chr_disconnect when error is not 
> > > EAGAIN.
> > > The new version will call tcp_chr_disconnect when size==-1. From the 
> > > tcp_chr_recv function we see EAGIN will return -1, so EAGIN will call 
> > > tcp_chr_disconnect.
> > > 
> > > We meet an issue when Guest VM use DPDK(1.6.0) l2fwd, it may exit 
> > > since link status is not up. The link is down because 
> > > tcp_chr_disconnect will set it.
> > > 
> > > Can you explain it why EAGIN here changes the behavior ?
> 
> > tcp_chr_read is used in a call to io_add_watch_poll() which sets up an event loop watch so that tcp_chr_read is called when there is incoming data to be read.  IOW, when tcp_chr_read is invoked, I would always expect that at least 1 byte is available, and so EAGAIN should not occurr.
> > 
> > So I'm puzzled why you would get EAGAIN at all, unless there is some kind of race with other code also consuming bytes from the same socket.
> 
> It could easily reproduce this when repeating 'rmmod virtio_net;modprobe virtio_net'.
> The call stack:
> #0  tcp_chr_disconnect (chr=0x7f09d5b2d540) at qemu-char.c:2867
> #1  0x00007f09d43b2106 in tcp_chr_read (chan=0x7f09d5b2df30, cond=G_IO_IN, opaque=0x7f09d5b2d540) at qemu-char.c:2917
> #2  0x00007f09d46364fa in qio_channel_fd_source_dispatch (source=0x7f093e603b30, callback=0x7f09d43b1fcb <tcp_chr_read>, user_data=0x7f09d5b2d540) at io/channel-watch.c:84
> #3  0x00007f09d2e9999a in g_main_context_dispatch () from /lib64/libglib-2.0.so.0
> #4  0x00007f09d45bfc96 in glib_pollfds_poll () at main-loop.c:213
> #5  0x00007f09d45bfd73 in os_host_main_loop_wait (timeout=991853) at main-loop.c:258
> #6  0x00007f09d45bfe23 in main_loop_wait (nonblocking=0) at main-loop.c:506
> #7  0x00007f09d43bbdd2 in main_loop () at vl.c:2119
> #8  0x00007f09d43c380e in main (argc=56, argv=0x7ffe7ad669a8, envp=0x7ffe7ad66b70) at vl.c:4896
> (gdb) f 1
> #1  0x00007f09d43b2106 in tcp_chr_read (chan=0x7f09d5b2df30, cond=G_IO_IN, opaque=0x7f09d5b2d540) at qemu-char.c:2917
> 2917	        tcp_chr_disconnect(chr);
> (gdb) p errno
> $1 = 11
> 
> EAGAIN = 11

I think what is happening is a race condition - the main event loop thread
sees I/O incoming triggering tcp_chr_read. Meanwhile the vhost-user.c file is
triggering vhost_user_read() in a different thread which consumes the incoming
I/O before tcp_chr_read can handle it.

IOW, you are right, we should have kept the EAGAIN handling code in tcp_chr_read
to avoid the disconnects.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|