Re: [Qemu-devel] [PATCH] monitor: Protect outbuf from concurrent access

[Jan Kiszka]

On 2011-09-02 11:41, Daniel P. Berrange wrote:
> On Thu, Sep 01, 2011 at 08:34:35PM -0500, Anthony Liguori wrote:
>> On 09/01/2011 02:35 PM, Luiz Capitulino wrote:
>>> Sometimes, when having lots of VMs running on a RHEV host and the user
>>> attempts to close a SPICE window, libvirt will get corrupted json from
>>> QEMU.
>>>
>>> After some investigation, I found out that the problem is that different
>>> SPICE threads are calling monitor functions (such as
>>> monitor_protocol_event()) in parallel which causes concurrent access
>>> to the monitor's internal buffer outbuf[].
>>>
>>> This fixes the problem by protecting accesses to outbuf[] with a mutex.
>>>
>>> Honestly speaking, I'm not completely sure this the best thing to do
>>> because the monitor itself and other qemu subsystems are not thread safe,
>>> so having subsystems like SPICE assuming the contrary seems a bit
>>> catastrophic to me...
>>>
>>> Anyways, this commit fixes the problem at hand.
>>
>> Nack.
>>
>> This is absolutely a Spice bug.  Spice should not be calling into
>> QEMU code from multiple threads.  It should only call into QEMU code
>> while it's holding the qemu_mutex.
>>
>> The right way to fix this is probably to make all of the
>> SpiceCoreInterface callbacks simply write to a file descriptor which
>> can then wake up QEMU to do the operation on behalf of it.   It's
>> ugly but the libspice interface is far too tied to QEMU internals in
>> the first place which is the root of the problem.
> 
> This feels like a rather short-term approach to fixing the problem
> to me. As QEMU becomes increasingly multi-threaded, there is high
> liklihood that we'll get other code in QEMU which wants to use the
> monitor from multiple threads. The monitor code in QEMU is fairly
> well isolated & thus comparatively easy to make threadsafe, so I

As pointed out before, this assumption is not correct.

> don't see why we wouldn't want todo that & avoid any chance of this
> type of problem recurring in the future.
> 
> IMHO, "fixing" SPICE is not fixing the bug at all, it is just removing
> the trigger of the bug in the monitor.

Until we have officially thread-safe subsystems, SPICE must take the
qemu_global_mutex before calling core services. This patch does not make
the monitor thread-safe as it does not address indirectly called services.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux