Re: [Qemu-devel] [PATCH v2] qemu-char: eliminate busy waiting on can_read returning zero

[Paolo Bonzini]

Il 06/04/2013 21:00, Amit Shah ha scritto:
> On (Fri) 05 Apr 2013 [17:59:33], Paolo Bonzini wrote:
>> The character backend refactoring introduced an undesirable busy wait.
>> The busy wait happens if can_read returns zero and there is data available
>> on the character device's file descriptor.  Then, the I/O watch will
>> fire continuously and, with TCG, the CPU thread will never run.
>>
>>     1) Char backend asks front end if it can write
>>     2) Front end says no
>>     3) poll() finds the char backend's descriptor is available
>>     4) Goto (1)
>>
>> What we really want is this (note that step 3 avoids the busy wait):
>>
>>     1) Char backend asks front end if it can write
>>     2) Front end says no
>>     3) poll() goes on without char backend's descriptor
>>     4) Goto (1) until qemu_chr_accept_input() called
>>
>>     5) Char backend asks front end if it can write
>>     6) Front end says yes
>>     7) poll() finds the char backend's descriptor is available
>>     8) Backend handler called
>>
>> After this patch, the IOWatchPoll source and the watch source are
>> separated.  The IOWatchPoll is simply a hook that runs during the prepare
>> phase on each main loop iteration.  The hook adds/removes the actual
>> source depending on the return value from can_read.
>>
>> A simple reproducer is
>>
>>     qemu-system-i386 -serial mon:stdio
>>
>> ... followed by banging on the terminal as much as you can. :)  Without
>> this patch, emulation will hang.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>         v1->v2: use g_source_get_context to find if the watch was active
> 
>>  static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_)
>>  {
>>      IOWatchPoll *iwp = io_watch_poll_from_source(source);
>> -
>> -    iwp->max_size = iwp->fd_can_read(iwp->opaque);
>> -    if (iwp->max_size == 0) {
>> +    bool now_active = iwp->fd_can_read(iwp->opaque) > 0;
>> +    bool was_active = g_source_get_context(iwp->src) != NULL;
> 
> This gives me a bunch of
> 
> (process:30075): GLib-CRITICAL **: g_source_get_context: assertion 
> `!SOURCE_DESTROYED (source)' failed
> 
> messages

This should fix it, but unfortunately Peter reports it is not enough:

diff --git a/qemu-char.c b/qemu-char.c
index 85ebcf9..c2fb985 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -610,8 +610,14 @@ static IOWatchPoll
*io_watch_poll_from_source(GSource *source)
 static gboolean io_watch_poll_prepare(GSource *source, gint *timeout_)
 {
     IOWatchPoll *iwp = io_watch_poll_from_source(source);
-    bool was_active = g_source_get_context(iwp->src) != NULL;
-    bool now_active = iwp->fd_can_read(iwp->opaque) > 0;
+    bool was_active, now_active;
+
+    if (g_source_is_destroyed(iwp->src)) {
+        return FALSE;
+    }
+
+    was_active = g_source_get_context(iwp->src) != NULL;
+    now_active = iwp->fd_can_read(iwp->opaque) > 0;
     if (was_active == now_active) {
         return FALSE;
     }


I'm not sure what is different between the microblaze and x86 cases.
Peter, please send us reproduction instructions.

Paolo