On Fri, Jun 15, 2018 at 02:37:49PM +0200, Markus Armbruster wrote: > Peter Xu <pet...@redhat.com> writes: > > > Out-Of-Band handlers need to protect shared state if there is any. > > Mention it in the document. > > > > Suggested-by: Markus Armbruster <arm...@redhat.com> > > Signed-off-by: Peter Xu <pet...@redhat.com> > > --- > > docs/devel/qapi-code-gen.txt | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt > > index 1366228b2a..bee9de35df 100644 > > --- a/docs/devel/qapi-code-gen.txt > > +++ b/docs/devel/qapi-code-gen.txt > > @@ -680,6 +680,9 @@ OOB command handlers must satisfy the following > > conditions: > Under normal QMP command execution, the following apply to each > command: > > - They are executed in order, > - They run only in main thread of QEMU, > - They have the BQL taken during execution. > > Not this patch's fault, but this sounds awkward. Perhaps "They run with > the BQL held." > > When a command is executed with OOB, the following changes occur: > > - They can be completed before a pending in-band command, > - They run in a dedicated monitor thread, > - They do not take the BQL during execution. > > "They run with the BQL not held". > > OOB command handlers must satisfy the following conditions: > > - It executes extremely fast, > > "It terminates quickly" > > - It does not take any lock, or, it can take very small locks if all > critical regions also follow the rules for OOB command handler code, > > "It takes only "fast" locks, i.e. all critical sections protected by any > lock it takes also satisfy the conditions for OOB command handler code." > Maybe make it the last item. > > > - It does not invoke system calls that may block, > > - It does not access guest RAM that may block when userfaultfd is > > enabled for postcopy live migration. > > All these are corollaries of the first item. But that's okay. > > > +- It needs to protect any shared state, since as long as a command > > + supports Out-Of-Band it means the handler can be run in parallel > > + with the same handler running in the other thread. > > "in another thread" > > Not just the same handler is a potential problem. Any code accessing > shared state from another thread is. > > "It needs" is not really a condition. > > Perhaps we can make this a separate paragraph rather than an additional > item: > > The restrictions on locking limit access to shared state. Such > access requires synchronization, but OOB commands can't take the BQL > or any other "slow" lock.
Yes this looks good to me. I'll apply the rest of suggestions in my next post along with this patch. I'll touch up the commit message a bit too. Thanks, -- Peter Xu
