From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58931) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eJGum-0002s1-NO for qemu-devel@nongnu.org; Mon, 27 Nov 2017 05:45:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eJGuf-0007uL-RW for qemu-devel@nongnu.org; Mon, 27 Nov 2017 05:45:28 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35746) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eJGuf-0007ry-Hr for qemu-devel@nongnu.org; Mon, 27 Nov 2017 05:45:21 -0500 Date: Mon, 27 Nov 2017 10:44:26 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20171127104426.GB2338@work-vm> References: <20171116130610.23582-1-peterx@redhat.com> <20171116130610.23582-22-peterx@redhat.com> <20171124131452.GD2302@work-vm> <20171127052003.GB5153@xz-mi> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171127052003.GB5153@xz-mi> Subject: Re: [Qemu-devel] [RFC v4 21/27] qmp: let migrate-incoming allow out-of-band List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Xu Cc: qemu-devel@nongnu.org, Stefan Hajnoczi , "Daniel P . Berrange" , Paolo Bonzini , Fam Zheng , Juan Quintela , mdroth@linux.vnet.ibm.com, Eric Blake , Laurent Vivier , Markus Armbruster , marcandre.lureau@redhat.com * Peter Xu (peterx@redhat.com) wrote: > On Fri, Nov 24, 2017 at 01:14:53PM +0000, Dr. David Alan Gilbert wrote: > > * Peter Xu (peterx@redhat.com) wrote: > > > So it can get rid of being run on main thread. > > > > > > Signed-off-by: Peter Xu > > > > Last time I asked if you were sure that we didn't do locking, > > and you explained that we end up just setting up a callback > > that happens in the mainloop, and this shouldn't take a lock. > > I confirmed this by: > > > > running with -incoming defer > > breakpointing in hmp_migrate_incoming > > doing migrate_incoming tcp:0:4444 > > once I hit that breakpointing adding two more breakpoints: > > a) qemu_mutex_lock_iothread > > b) the end of hmp's handle_hmp_command > > > > and indeed it hit the end of handle_hmp_command without > > having hit the qemu_mutex_lock_iothread; so initially that > > looks ok. > > I am not sure I fully understand the test above - I think it was > trying to verify the whole OOB thing running without BQL? If so, > there are possibly two things missing: > > Firstly, qemu_mutex_lock_iothread() is actually called before we call > hmp_migrate_incoming(). To verify it is simple: just break at entry of > hmp_migrate_incoming() and do "p iothread_locked", it'll be true (I > would always prefer printing that global variable to know whether > current thread is in a BQL section). > Yes, that's a good point - I was really trying to just follow through qmp_migrate_incoming a bit, but you've got a point that it was really the wrong way to look at it. > OOB for QMP command "migrate-incoming" rather than the HMP command. So > IMHO what we need to test is QMP command, rather than this HMP one. > > To test that QMP command, it's still not that easy (actually awkward). > We need to first enable "OOB" when doing handshake for QMP: > > {"execute": "qmp_capabilities", "arguments": { "enable": [ "oob" ] } } > > Then, we need to send the command with proper "id"/"control" field: > > { "execute": "migrate-incoming", > "arguments": { "uri": "tcp:localhost:1234" }, > "control": { "run-oob": true }, > "id": "test-command" } > > Note that here "id" field will be a must now since OOB requires that, > meanwhile the "control" field is a must too to make sure that is run > in OOB format (otherwise this command will still take BQL and run as > usual). So if you see we do have a lot of protection to make sure we > only run OOB only if we really wanted to... otherwise we'll always run > in compatible and old way. > > This time if we break at qmp_migrate_incoming() and then do "p > iothread_locked", we should see a false here. > > > > > But then I added a break on pthread_mutex_lock, and I've got > > this set caused by qemu_start_incoming_migration sending a > > MIGRATION_STATUS_SETUP event: > > > > #1 0x0000555555ba6eba in qemu_mutex_lock (mutex=mutex@entry=0x5555563f9ba0 ) > > at /home/dgilbert/git/qemu/util/qemu-thread-posix.c:65 > > #2 0x00005555557f01c1 in monitor_qapi_event_queue (event=QAPI_EVENT_MIGRATION, qdict=0x555557d93c00, errp=) at /home/dgilbert/git/qemu/monitor.c:442 > > > > 440 trace_monitor_protocol_event_queue(event, qdict, evconf->rate); > > 441 > > 442 qemu_mutex_lock(&monitor_lock); > > 443 > > > > #3 0x0000555555b92722 in qapi_event_send_migration (status=status@entry=MIGRATION_STATUS_SETUP, errp=0x555556859320 ) at qapi-event.c:661 > > #4 0x0000555555a6159f in qemu_start_incoming_migration (uri=0x555557693f30 "tcp:0:4444", errp=0x7fffffffc700) > > at /home/dgilbert/git/qemu/migration/migration.c:253 > > #5 0x0000555555a641c5 in qmp_migrate_incoming (uri=0x555557693f30 "tcp:0:4444", errp=0x7fffffffc730) > > at /home/dgilbert/git/qemu/migration/migration.c:1321 > > > > is there anything which protects us there? > > IIUC you mean what if we e.g. page fault during taking the > monitor_lock? IMHO it just can't happen - monitor_lock is really used > in limited places and during those critical sections there is no guest > memory access at all (which only protects the monitor logic itself > AFAICT). OK, so we should document somewhere which locks it's OK to take in an OOB command, and then make sure that for each of those locks we add a note saying that anyone taking them must be careful. We should also add a note above teh qmp_migrate_incoming that it's an OOB command and to take care. However, since you've convinced me it's OK: Reviewed-by: Dr. David Alan Gilbert > Thanks, > > > > > Dave > > > > > --- > > > qapi/migration.json | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/qapi/migration.json b/qapi/migration.json > > > index bbc4671ded..95098072dd 100644 > > > --- a/qapi/migration.json > > > +++ b/qapi/migration.json > > > @@ -1063,7 +1063,8 @@ > > > # <- { "return": {} } > > > # > > > ## > > > -{ 'command': 'migrate-incoming', 'data': {'uri': 'str' } } > > > +{ 'command': 'migrate-incoming', 'data': {'uri': 'str' }, > > > + 'allow-oob': true } > > > > > > ## > > > # @xen-save-devices-state: > > > -- > > > 2.13.6 > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > -- > Peter Xu -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK