From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47753) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dk4rU-0001sB-6E for qemu-devel@nongnu.org; Tue, 22 Aug 2017 04:48:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dk4rR-0004T9-3o for qemu-devel@nongnu.org; Tue, 22 Aug 2017 04:48:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38426) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dk4rQ-0004Qv-PX for qemu-devel@nongnu.org; Tue, 22 Aug 2017 04:48:33 -0400 Date: Tue, 22 Aug 2017 16:48:25 +0800 From: Fam Zheng Message-ID: <20170822084825.GA3210@lemon> References: <20170821135743.GC4371@lemon> <20170821153622.GG2231@work-vm> <20170821165450.GE4371@lemon> <20170821172852.GA3236@work-vm> <20170822021556.GA2146@lemon> <20170822025654.GE30356@pxdev.xzpeter.org> <20170822041519.GC2146@lemon> <20170822055903.GF30356@pxdev.xzpeter.org> <20170822063348.GE2146@lemon> <20170822082940.GB2109@work-vm> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170822082940.GB2109@work-vm> Subject: Re: [Qemu-devel] [RFC 0/6] monitor: allow per-monitor thread List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: Peter Xu , Laurent Vivier , Juan Quintela , Markus Armbruster , mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, Paolo Bonzini On Tue, 08/22 09:29, Dr. David Alan Gilbert wrote: > * Fam Zheng (famz@redhat.com) 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