Re: [Qemu-devel] [RFC PATCH] main-loop: Unconditionally unlock iothread

[Paolo Bonzini]

Il 02/04/2013 11:04, Peter Crosthwaite ha scritto:
> Public bug: 1154328
> Broken Commit: a29753f8aa79a34a324afebe340182a51a5aef11
> 
> ATM, the timeout from g_pollfds_fill is inhibiting unlocking of the
> iothread. This is capable of causing a total deadlock when hw/serial
> is used as a device. The bug manifests when you go -nographic -serial
> mon:stdio and then paste 40 or more chars into the terminal.
> 
> My knowledge of this g_foo is vague at best, but my best working
> theory is this:
> 
> - First 8 chars are recieved by the serial device no complaints.
> - The next 32 chars, serial returns false for can_receive() so they
>       are buffered by the MuxDriver object - mux_chr_read()
> - Buffer is full, so 41st char causes false return from Muxes own
>       can_read()
> - This propagates all the way up to glib_pollfds_fill and manifests
>       as a timeout

I suppose you mean "manifests as timeout==0".  The question is *which*
GSource has a timeout of zero?  Not the mux's: if mux_chr_can_read()
returns zero, the prepare function returns FALSE without touching the
timeout at all...

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) {
        return FALSE;
    }

    return g_io_watch_funcs.prepare(source, timeout_);
}

> - Timeout means no unlock of IOthread. Device land never sees any more
>       cycles so the serial port never progresses - no flushing of
>       buffer

Still, this is plausible, so the patch looks correct.

Paolo

> - Deadlock
> 
> Tested on petalogix_ml605 microblazeel machine model, which was faulty
> due to 1154328.
> 
> Fix by removing the conditions on unlocking the iothread. Don't know
> what else this will break but the timeout is certainly the wrong
> condition for the unlock. Probably the real solution is to have a more
> selective unlock policy.
> 
> I'm happy for someone to take this patch off my hands, or educate me on
> the correct implementation. For the peeps doing automated testing on
> nographic platforms this will get your build working again.
> 
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
>  main-loop.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/main-loop.c b/main-loop.c
> index eb80ff3..a376898 100644
> --- a/main-loop.c
> +++ b/main-loop.c
> @@ -194,15 +194,11 @@ static int os_host_main_loop_wait(uint32_t timeout)
>  
>      glib_pollfds_fill(&timeout);
>  
> -    if (timeout > 0) {
> -        qemu_mutex_unlock_iothread();
> -    }
> +    qemu_mutex_unlock_iothread();
>  
>      ret = g_poll((GPollFD *)gpollfds->data, gpollfds->len, timeout);
>  
> -    if (timeout > 0) {
> -        qemu_mutex_lock_iothread();
> -    }
> +    qemu_mutex_lock_iothread();
>  
>      glib_pollfds_poll();
>      return ret;
>