Re: [Qemu-devel] [PATCH v3 4/6] docs: mention shared state protect for OOB

[Peter Xu]

On Fri, Jun 15, 2018 at 02:37:49PM +0200, Markus Armbruster wrote:
> Peter Xu <address@hidden> writes:
> 
> > Out-Of-Band handlers need to protect shared state if there is any.
> > Mention it in the document.
> >
> > Suggested-by: Markus Armbruster <address@hidden>
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  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