| Message ID | BLUPR0301MB2034FD833FD5B3E2D7F6E4D59EB50@BLUPR0301MB2034.namprd03.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 11, 2016 at 11:51:29PM +0000, Andrew Baumann wrote: > Hi folks, > > > From: Paolo Bonzini [mailto:pbonzini@redhat.com] > > Sent: Thursday, 10 March 2016 9:37 AM > > > > On 10/03/2016 18:26, Daniel P. Berrange wrote: > > > This series started out as an attempt to fix the Win32 problems > > > identified by Andrew Baumann > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg01343.html > > > > > > It turned into a significantly larger cleanup of some chardev > > > and osdep win32 portability code. > [...] > > Sorry for chiming in a bit late here. I've tested these patches > (the complete set, not individually), and they do appear to fix my > immediate problem: socket char devices now work again. So thank you! Thanks for confirming this, these patches have now merged into git msater. > However, I'm now seeing a problem I don't believe we had before: > very slow responses to GDB commands. From looking at a packet > capture (using a localhost tcp socket between qemu and my gdb > client), it seems that a couple of operations will go through > just fine, and then there is a 1 second delay between my client's > request and qemu's response. After fiddling with poll timeouts, > it became clear that we were noticing the socket events when > waking up from the poll, but the events themselves were still > not waking us. It turns out that we were not calling WSAEventSelect > on the accept path. At least, the following patch fixed the > problem for me: > > diff --git a/qemu-char.c b/qemu-char.c > index 3bf30b5..c1be622 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -3047,6 +3047,7 @@ static gboolean tcp_chr_accept(QIOChannel *channel, > return TRUE; > } > > + qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL); > tcp_chr_new_client(chr, sioc); > > object_unref(OBJECT(sioc)); > > However, I'd note that both callers of tcp_chr_new_client() > make the same call to set blocking to false immediately before > calling tcp_chr_new_client(). Furthermore, the doc comment for > qio_channel_set_blocking() appears to suggest that non-blocking > mode is the default. If that's true, maybe you don't even want > to rely on the caller explicitly setting blocking to false? No, the docs don't intend to suggest that - the default is in fact blocking mode, so its correct to place it into nonblocking mode explicitly. I think I didn't notice the problem you describe because my original patch series had us call WSAEventSelect when creating the watch. This indirectly puts Win32 sockets into non-blocking mode. The patches which just merged however no longer call WSAEventSelect when creating the watch, instead requiring the caller to explicitly set the socket into non-blocking mode. So I think your suggested addition here is probably the right way to address this. I'll investigate and respond with a followup patch as needed. Regards, Daniel
diff --git a/qemu-char.c b/qemu-char.c index 3bf30b5..c1be622 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -3047,6 +3047,7 @@ static gboolean tcp_chr_accept(QIOChannel *channel, return TRUE; } + qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL); tcp_chr_new_client(chr, sioc); object_unref(OBJECT(sioc));