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