From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35267) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UNnWN-00069H-0q for qemu-devel@nongnu.org; Thu, 04 Apr 2013 13:00:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UNnWD-0003yh-3I for qemu-devel@nongnu.org; Thu, 04 Apr 2013 13:00:18 -0400 Received: from e23smtp08.au.ibm.com ([202.81.31.141]:47429) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UNnWC-0003uL-Ig for qemu-devel@nongnu.org; Thu, 04 Apr 2013 13:00:09 -0400 Received: from /spool/local by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 5 Apr 2013 02:57:50 +1000 Received: from d23relay04.au.ibm.com (d23relay04.au.ibm.com [9.190.234.120]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id C4C03357804A for ; Fri, 5 Apr 2013 03:59:52 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r34GkAwl10748364 for ; Fri, 5 Apr 2013 03:46:11 +1100 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r34GxK6X018315 for ; Fri, 5 Apr 2013 03:59:21 +1100 From: Anthony Liguori In-Reply-To: <515D135B.4020003@redhat.com> References: <1364893452-10604-1-git-send-email-peter.crosthwaite@xilinx.com> <515ABCD1.2070008@redhat.com> <82877867.1048290.1364970927865.JavaMail.root@redhat.com> <515D135B.4020003@redhat.com> Date: Thu, 04 Apr 2013 11:59:14 -0500 Message-ID: <87mwteteot.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [RFC PATCH] main-loop: Unconditionally unlock iothread List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Peter Crosthwaite Cc: Amit Shah , qemu-devel@nongnu.org, gson@gson.org Paolo Bonzini writes: > Il 04/04/2013 01:58, Peter Crosthwaite ha scritto: >> >> I think there may be a flaw in that "any of the descriptors being >> pollable" is not a good definition of progress. stdin is blocked by >> the fact that the device and mux cannot accept their data anymore so >> even though its readable, no meaningful read will happen. That leaves >> us with having to devise more elaborate code to define progress, or we >> simplify by just removing this nonblocking optimisation altogether >> (original patch). > > If stdin is blocked, it shouldn't be polled at all. That is the purpose > of the can_read callback. Unfortunately, return FALSE from the prepare > callback still leaves the poll handler. > > So your original patch fixes the symptom, but leaves the busy waiting > unfixed. > > The right thing to use would be g_source_add_child_source() and > g_source_remove_child_source(), but that is only present since glib 2.28 > and we currently require 2.12 (2.20 on Windows). I don't think a child source fixes the problem. The backend definitely has work to do. What we don't know is whether the front end is capable of processing the work. The problem here is that we use polling on the front-end. IOW: 1) Char backend has data and is ready to write. 2) Asks front end if it can write 3) Front end says no 4) Goto (1) It does this without dropping the BQL which means that TCG never gets to run. Ideally, we would do: 1) Char backend has data and is ready to write. 2) Asks front end if it can write 3) Backend stops asking front end until front end says it can receive data again 4) Drop BQL because we have no more work to do. But this requires eliminating all users of can_read() which isn't going to happen any time soon. The solution therefore needs to be: 1) Char backend has data and is ready to write. 2) Asks front end if it can write 3) Front end says no 3.5) Drop the BQL and give the VCPU a chance to run 4) Goto (1) Which is essentially what Peter's patch does. In fact, you can imagine other scenarios where a very busy VNC client essentially blocks the BQL. We need to regularly give up the BQL in order to prevent starvation regardless of the polling in the char layer. So I think this is a long way of saying: Reviewed-by: Anthony Liguori Regards, Anthony Liguori > > Anthony, Amit, can you look at it? > > Paolo