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

[Peter Xu]

On Fri, Dec 06, 2024 at 07:40:19PM +0100, Maciej S. Szmigiero wrote:
> 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.

Yeah, I'd go for it if I'm touching it.  But it's your call, either use
this (it'll need 5 extra minutes for me to review such, but it's ok), or go
with what I said, or drop this patch and leave it for later.

-- 
Peter Xu