Re: [PATCH v3 07/24] migration: Document the BQL behavior of load SaveVMHandlers

[Maciej S. Szmigiero]

On 4.12.2024 22:38, Peter Xu wrote:
> On Sun, Nov 17, 2024 at 08:20:02PM +0100, Maciej S. Szmigiero wrote:
> > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> >
> > Some of these SaveVMHandlers were missing the BQL behavior annotation,
> > making people wonder what it exactly is.
> >
> > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> > ---
> >   include/migration/register.h | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/include/migration/register.h b/include/migration/register.h
> > index 39991f3cc5d0..761e4e4d8bcb 100644
> > --- a/include/migration/register.h
> > +++ b/include/migration/register.h
> > @@ -212,6 +212,8 @@ typedef struct SaveVMHandlers {
> >       void (*state_pending_exact)(void *opaque, uint64_t *must_precopy,
> >                                   uint64_t *can_postcopy);
> >   
> > +    /* This runs inside the BQL. */
> > +
> >       /**
> >        * @load_state
> >        *
> > @@ -246,6 +248,8 @@ typedef struct SaveVMHandlers {
> >       int (*load_state_buffer)(void *opaque, char *buf, size_t len,
> >                                Error **errp);
> >   
> > +    /* The following handlers run inside the BQL. */
> > +
> >       /**
> >        * @load_setup
> >        *
> > @@ -272,6 +276,9 @@ typedef struct SaveVMHandlers {
> >        */
> >       int (*load_cleanup)(void *opaque);
> >   
> > +
> > +    /* This runs outside the BQL. */
> > +
> >       /**
> >        * @resume_prepare
> >        *
> > @@ -284,6 +291,8 @@ typedef struct SaveVMHandlers {
> >        */
> >       int (*resume_prepare)(MigrationState *s, void *opaque);
> >   
> > +    /* The following handlers run inside the BQL. */
> > +
> >       /**
> >        * @switchover_ack_needed
> >        *
>
> Such change is not only error prone when adding new hooks, it's also hard
> to review..
>
> If we do care about that, I suggest we attach that info to every command.
> For example, changing from:
>
>      /**
>       * @save_state
>       * ...
>
> To:
>
>      /**
>       * @save_state (invoked with BQL)
>       * ...
>
> Or somewhere in the doc lines of each hook.

This would need rewriting all the existing BQL comments/annotations
in SaveVMHandlers since all of these are of the "separator" form as
these introduced in this patch.

Thanks,
Maciej