[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread |
Date: |
Tue, 22 Aug 2017 09:29:43 +0100 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
* 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.
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).
Dave
> Fam
>
> >
> > 3. which monitor commands can be run without BQL?
> >
> > This is what patch 4/6 was doing. It tries to move
> > migrate_incoming command out as the first candidate BQL-free
> > command.
> >
> > Yes it's hard to say which command can be run without BQL. So we
> > need to investigate, possibly modify existing codes to make sure
> > it's thread-safe, prove validity, then we can add the new ones into
> > the BQL-free list.
> >
> > If after evaluating the pros and cons, we found that one command
> > can be put into BQL-free but not worth the time for working on it,
> > we can also keep those commands under BQL.
> >
> > I assume question 3 is the one you were asking, and I'd say we may
> > need to solve question 1/2 first. If we are done with 1/2, we just
> > need to spend time on each command to prove whether it is doable to
> > let that command run without BQL, and whether it worths itself to move
> > the command out of BQL. Then we decide. Thanks,
> >
> > --
> > Peter Xu
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, (continued)
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, Fam Zheng, 2017/08/21
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, Dr. David Alan Gilbert, 2017/08/21
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, Fam Zheng, 2017/08/21
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, Dr. David Alan Gilbert, 2017/08/21
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, Fam Zheng, 2017/08/21
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, Peter Xu, 2017/08/21
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, Fam Zheng, 2017/08/22
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, Peter Xu, 2017/08/22
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, Fam Zheng, 2017/08/22
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, Peter Xu, 2017/08/22
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread,
Dr. David Alan Gilbert <=
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, Fam Zheng, 2017/08/22
- Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, Dr. David Alan Gilbert, 2017/08/22
Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread, no-reply, 2017/08/22