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

[Peter Xu]

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...  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.

> 
> However, since you've convinced me it's OK:
> 
> 
> Reviewed-by: Dr. David Alan Gilbert <address@hidden>

Thanks!

-- 
Peter Xu