Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread

[Fam Zheng]

On Tue, 08/22 09:29, Dr. David Alan Gilbert wrote:
> * Fam Zheng (address@hidden) wrote:
> > On Tue, 08/22 13:59, Peter Xu wrote:
> > > On Tue, Aug 22, 2017 at 12:15:19PM +0800, Fam Zheng wrote:
> > > > On Tue, 08/22 10:56, Peter Xu wrote:
> > > > > I haven't really encountered (c), but I think it's the migrate_cancel
> > > > > command that matters, which should not need BQL as well.
> > > > 
> > > > There is bdrv_invalidate_cache_all() in migrate_cancel which clearly 
> > > > isn't safe.
> > > > Is that if block unreachable in this case? If so we should assert, 
> > > > otherwise
> > > > this command is not okay to run without BQL.
> > > 
> > > Ah. I see.  Even if so, if that is the only usage of BQL, IMHO we can
> > > still mark migrate_cancel as "without-bql=true", instead we take the
> > > BQL before calling bdrv_invalidate_cache_all().  Then migrate_cancel
> > > can be BQL-free at least when block migration is not active.
> > > 
> > > > 
> > > > Generically, what guarantee the thread-safety of a qmp command when you 
> > > > decide
> > > > BQL is not needed? In other words, how do you prove commands are safe 
> > > > without
> > > > BQL? I think almost every command accesses global state, but lock-free 
> > > > data
> > > > structures are rare AFAICT.
> > > 
> > > I would suggest we split the problem into at least three parts.  IMHO
> > > we need to answer below questions one by one to know what we should do
> > > next:
> > > 
> > > 1. whether we can handle monitor commands outside iothread, or say, in
> > >    an isolated thread?
> > > 
> > >    This is basically what patch 2 does, the "per-monitor threads".
> > > 
> > >    IMHO this is the very first question to ask.  So now I know that at
> > >    least current code cannot do it.  We need to at least do something
> > >    to remove/replace the assertion to make this happen.  Can we?  I
> > >    don't really know the answer yet.  If this is undoable, we can skip
> > >    question 2/3 below and may need to rethink on how to solve the
> > >    problem that postcopy recovery encounters.
> > > 
> > > 2. whether there is any monitor commands can run without BQL?
> > > 
> > >    This is basically what patch 3/5 does, one for QMP, one for HMP.
> > > 
> > >    If we can settle question 1, then we can possibly start consider
> > >    this question.  This step does not really allow any command to run
> > >    without BQL, but we need to know whether it's possible in general,
> > >    and if possible, we provide a framework to allow QMP/HMP developers
> > >    to specify that.  If you see patch 3/5, the default behavior is
> > >    still taking the BQL for all commands.
> > > 
> > >    IMHO doing this whole thing is generally good in the sense that
> > >    this is actually forcing ourselves to break the BQL into smaller
> > >    locks.  Take the migration commands for example: migrate_incoming
> > >    do not need BQL, and when we write codes around it we know that we
> > >    don't need to think about thread-safety.  That's not good IMHO.  I
> > >    think it's time we should start consider thread-safety always.
> > >    Again, for migrate_incoming to do this, actually we'll possibly at
> > >    least need a migration management lock (the smaller lock) to make
> > >    sure e.g. the user is not running two migrate_incoming commands in
> > >    parallel (after per-monitor threads, it can happen).  But it's
> > >    better than BQL, because BQL is for sure too big, so even a guest
> > >    page access (as long as it held the BQL) can block migration
> > >    commands.
> > 
> > Yes, this is my point. You cannot just declare a command "BQL-free" without
> > adding small locks first, and I think this is actually missing in this 
> > series.
> > As you said, two per-monitor threads can race if they do migrate_incoming in
> > parallel.  This is also the answer to 3.
> 
> Some commands really could be lock-free.  For example an 'info status'
> should just be a read without any locking.

Indeed, it makes sense to enable as many info (query-*) commands as possible on
non-mainloop monitors.

> It might also be possible to make a version of migrate_cancel lock-free;
> in particular the critical part is being able to do a shutdown() on the
> socket - that's enough to cause any of the migration threads to 
> unblock and take their failure path.
> 
> Also in the case of COLO; there should be an equivalent place to
> do the shutdown() on the migration thread and then set a flag to cause
> failover.
> (It's currently qmp_x_colo_lost_heartbeat but I think the eventual
> version would be more complex).

Fam