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

[Peter Xu]

On Mon, Nov 27, 2017 at 11:04:00AM +0000, Dr. David Alan Gilbert wrote:
> * 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.

Right.  That sounds like a HOWTO to migrate existing commands to
support OOB.  Hmm, could there be any/much?  I'm curious.

Anyways... how about I add this paragraph to doc patch?

  Converting an existing QMP command to be OOB-capable can be either
  very easy or extremely hard.  The most important thing is that, the
  command should never be blocked unexpectedly in any form.  For
  example, it's still legal to take a mutex in an OOB-capable command
  handler (but never the BQL!), but we need to make sure that all the
  other code paths that are protected by the same lock won't be
  blocked too.  Some candidates that can easily block: (1) doing IOs
  (especially when the backend can be an NFS storage), or (2)
  accessing guest memories (which can be simply blocked when it
  triggers a page fault, and which cannot be handled immediately).

Thanks,

-- Peter Xu