Re: [Qemu-devel] [RFC v4 21/27] qmp: let migrate-incoming allow out-of-band

[Dr. David Alan Gilbert]

* Peter Xu (address@hidden) wrote:
> On Mon, Nov 27, 2017 at 10:44:26AM +0000, Dr. David Alan Gilbert wrote:
> 
> [...]
> 
> > > > 
> > > > 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 (address@hidden 
> > > > <monitor_lock>)
> > > >     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=<optimized 
> > > > out>) 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 (address@hidden, 
> > > > errp=0x555556859320 <error_abort>) 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.
> 
> Makes sense.  I think it will be hard to document what locks can be
> taken during OOB command handing since there can be too many locks
> there...

Yes, but we need people who are marking commands as OOB to really think
about it, otherwise they'll use a lock that in some weird corner case
can also be taken by another command that can block.

> But at least I can add a comment to qmp_migrate_incoming()
> along with current patch to note that this function should be OOB
> friendly from now on.

Thanks.

Dave
 
> > 
> > However, since you've convinced me it's OK:
> > 
> > 
> > Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> 
> Thanks!
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK