On Wed, Jul 18, 2018 at 05:38:11PM +0200, Markus Armbruster wrote: > Peter Xu <pet...@redhat.com> writes: > > > After the Out-Of-Band work, the monitor iothread may be accessing the > > cur_mon as well (via monitor_qmp_dispatch_one()). Let's convert the > > cur_mon variable to be a per-thread variable to make sure there won't be > > a race between threads when accessing the variable. > > Hmm... why hasn't the OOB work created such a race already? > > A monitor reads, parses, dispatches and executes commands, formats and > sends replies. > > Before OOB, all of that ran in the main thread. Any access of cur_mon > should therefore be from the main thread. No races. > > OOB moves read, parse, format and send to an I/O thread. Dispatch and > execute remain in the main thread. *Except* for commands executed OOB, > dispatch and execute move to the I/O thread, too. > > Why is this not racy? I guess it relies on careful non-use of cur_mon > in any part that may now execute in the I/O thread. Scary...
I think it's because cur_mon is not really used in out-of-band command executions - now we only have a few out-of-band enabled commands, and IIUC none of them is using cur_mon (for example, in qmp_migrate_recover() we don't even call error_report, and the code path is quite straight forward to make sure of that). So IIUC cur_mon variable is still only touched by main thread for now hence we should be safe. However that condition might change in the future when we add more out-of-band capable commands. (not to mention that I don't even know whether there are real users of out-of-band if we haven't yet started to support that for libvirt...) > > Should this go into 3.0 to reduce the risk of bugs? Yes I think it would be good to have that even for 3.0, since it still can be seen as a bug fix of existing code. Regards, -- Peter Xu
