From mboxrd@z Thu Jan 1 00:00:00 1970 From: Amit Shah Subject: Re: PATCH: virtio_console: Fix poll blocking even though there is data to read Date: Thu, 16 Sep 2010 11:52:01 +0530 Message-ID: <20100916062201.GC2429@amit-laptop.redhat.com> References: <4C90C475.4010809@redhat.com> <4C90CC11.70202@redhat.com> <20100915134624.GB12320@amit-laptop.redhat.com> <201009161532.55462.rusty@rustcorp.com.au> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Hans de Goede , virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, spice-devel@lists.freedesktop.org, Michael Kerrisk To: Rusty Russell Return-path: Received: from mx1.redhat.com ([209.132.183.28]:18812 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753816Ab0IPGWT (ORCPT ); Thu, 16 Sep 2010 02:22:19 -0400 Content-Disposition: inline In-Reply-To: <201009161532.55462.rusty@rustcorp.com.au> Sender: kvm-owner@vger.kernel.org List-ID: On (Thu) Sep 16 2010 [15:32:54], Rusty Russell wrote: > On Wed, 15 Sep 2010 11:16:24 pm Amit Shah wrote: > > On (Wed) Sep 15 2010 [15:37:21], Hans de Goede wrote: > > > >>--- linux-2.6.35.x86_64/drivers/char/virtio_console.c~ 2010-08-= 02 00:11:14.000000000 +0200 > > > >>+++ linux-2.6.35.x86_64/drivers/char/virtio_console.c 2010-09-1= 5 13:39:29.043505000 +0200 > > > >>@@ -642,7 +642,7 @@ static unsigned int port_fops_poll(struc > > > >> poll_wait(filp,&port->waitqueue, wait); > > > >> > > > >> ret =3D 0; > > > >>- if (port->inbuf) > > > >>+ if (!will_read_block(port)) > > > > > > > >Looks correct, but this should be > > > > > > > > if (port_has_data(port)) > > > > > > > >instead. > > >=20 > > > That certainly works for me (as in will still fix the bug I'm hit= ting), but > > > quoting from "man 2 select": > > >=20 > > > Three independent sets of file descriptors are watched. = Those listed > > > in readfds will be watched to see if characters become a= vailable for > > > reading (more precisely, to see if a read will not block= ; in particu=E2=80=90 > > > lar, a file descriptor is also ready on end-of-file) > > >=20 > > > Notice the "a file descriptor is also ready on end-of-file", and > > > port_fops_read treats the host not being connected as eof (it ret= urns 0 > > > in that case). So from an API pov I'm not sure what is correct. > >=20 > > poll(2) says: > >=20 > > POLLIN There is data to read. > >=20 > > That makes it simple. >=20 > That's a documentation bug. On a pipe, POLLIN is set when the other = end > closes. read() then returns 0 immediately. Currently we don't set POLLIN when host goes down. I'll do a second patch for that. > poll() sets POLLIN when read() won't block, and people count on it. Yes, that's the behaviour with Hans's new patch as well -- that's not changing. The will_read_block() function (and the comment on top of it) are causing this confusion: /* The condition that must be true for polling to end */ static bool will_read_block(struct port *port) { if (!port->guest_connected) { /* Port got hot-unplugged. Let's exit. */ return false; } return !port_has_data(port) && port->host_connected; } This function is only called to unblock a blocking read() call. So the comment there has to be changed to read 'waiting' to end instead of 'polling' to end. read() does return 0 immediately when the other end is not connected (and there's no data to read). In effect, we need: - Hans's patch - a patch to set POLLIN when host goes down (in addition to POLLHUP and SIGIO) - a patch to change the comment for will_read_block. Thanks, Amit