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

[Markus Armbruster <armbru@redhat.com>]

Peter Xu <peterx@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 <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@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.

>  If in doubt, do not implement OOB execution support.