Re: A bug of Monitor Chardev ?

[Daniel P . Berrangé]

On Fri, May 21, 2021 at 01:09:17PM -0400, Peter Xu wrote:
> On Fri, May 21, 2021 at 05:56:14PM +0100, Daniel P. Berrangé wrote:
> > On Fri, May 21, 2021 at 05:33:46PM +0100, Daniel P. Berrangé wrote:
> > > On Fri, May 21, 2021 at 10:43:36AM -0400, Peter Xu wrote:
> > > > 
> > > > I think the original problem was that if qemu_chr_fe_set_handlers() is 
> > > > called
> > > > in main thread, it can start to race somehow within execution of the 
> > > > function
> > > > qemu_chr_fe_set_handlers() right after we switch context at:
> > > > 
> > > >     qemu_chr_be_update_read_handlers(s, context);
> > > > 
> > > > Then the rest code in qemu_chr_fe_set_handlers() will continue to run 
> > > > in main
> > > > thread for sure, but the should be running with the new iothread 
> > > > context, which
> > > > introduce a race condition.
> > > > 
> > > > Running qemu_chr_be_update_read_handlers() in BH resolves that because 
> > > > then all
> > > > things run in the monitor iothread only and natually serialized.
> > > 
> > > The first message in this thread, however, claims that it is *not*
> > > in fact serialized, when using the BH. 
> > > 
> > > > So the new comment looks indeed not fully right, as the chr device 
> > > > should be
> > > > indeed within main thread context before qemu_chr_fe_set_handlers(), 
> > > > it's just
> > > > that the race may start right away if without BH when context switch 
> > > > happens
> > > > for the chr.
> > > 
> > > It sounds like both the comment and the code are potentially wrong.
> > 
> > 
> > I feel like our root cause problem that the original code was trying to
> > workaround, is that the chardev is "active" from the very moment it is
> > created, regardless of whether the frontend is ready to use it.
> > 
> > IIUC, in this case the socket chardev is already listen()ing and
> > accept()ing incoming clients off the network, before the monitor
> > has finished configuring its hooks into the chardev. This means
> > that the initial listen()/accept() I/O watches are using the
> > default GMainContext, and the monitor *has* to remove them and
> > put in new watches on the thread private GMainContext.
> > 
> > To eliminate any risk of races, we need to make it possible for the
> > monitor to configure the GMainContext on the chardevs *before* any
> > I/O watches are configured.
> > 
> > This in turn suggests that we need to split the chardev initialization
> > into two phases. First we have the basic chardev creation, with object
> > creation, option parsing/sanity checking, socket creation, and then
> > second we have the actual activation where the I/O watches are added.
> 
> When we are still running the monitor_init() code IIUC the main thread is 
> still
> occupied so it won't be able to handle any listen()/accept() even if there's
> event already, so IIUC race can only happen when main thread started the event
> loop then run concurrently with the BH in the other iothread.
> 
> So, besides above split of chardev init (which sounds like a bigger
> surgery)... would it also work if we let the main thread wait until that BH
> executed in monitor iothread?  Then all pending listen()/accept() event will
> directly be done in the monitor thread.

My concern is what happens when we add support for monitor hotplug
in the future. A client wanting to add a second monitor to a running
QEMU will run "chardev-add" followed by a hypothetical "monitor-add"
command. Both of these will be processed in the monitor thread, so
the main thread will have already started handling I/O events for
the chardev before "monitor-add" gets processed. Thus I think the
only long term safe solution is to have a design that guarantees
no I/O events are registered by *any* chardev impl, until the
frontend connects to the chardev.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|