* Peter Xu (pet...@redhat.com) 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 > > > > (mutex=mutex@entry=0x5555563f9ba0 <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 > > > > (status=status@entry=MIGRATION_STATUS_SETUP, 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 <dgilb...@redhat.com> > > Thanks! > > -- > Peter Xu -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
