Re: [PATCH v3 18/24] vfio/migration: Don't run load cleanup if load setup didn't run

[Maciej S. Szmigiero]

On 3.12.2024 16:09, Avihai Horon wrote:
> On 29/11/2024 19:15, Maciej S. Szmigiero wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On 29.11.2024 15:08, Cédric Le Goater wrote:
> > > On 11/17/24 20:20, Maciej S. Szmigiero wrote:
> > > > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> > > >
> > > > It's possible for load_cleanup SaveVMHandler to get called without
> > > > load_setup handler being called first.
> > > >
> > > > Since we'll be soon running cleanup operations there that access objects
> > > > that need earlier initialization in load_setup let's make sure these
> > > > cleanups only run when load_setup handler had indeed been called
> > > > earlier.
> > > >
> > > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> > >
> > > tbh, that's a bit ugly. I agree it's similar to those 'bool initialized'
> > > attributes we have in some structs, so nothing new or really wrong.
> > > But it does look like a workaound for a problem or cleanups missing
> > > that would need time to untangle.
> > >
> > > I would prefer to avoid this change and address the issue from the
> > > migration subsystem if possible.
> >
> > While it would be pretty simple to only call {load,save}_cleanup
> > SaveVMHandlers when the relevant {load,save}_setup handler was
> > successfully called first this would amount to a change of these
> > handler semantics.
> >
> > This would risk introducing regressions - for example vfio_save_setup()
> > doesn't clean up (free) newly allocated migration->data_buffer
> > if vfio_migration_set_state() were to fail later in this handler
> > and relies on an unconstitutional call to vfio_save_cleanup() in
> > order to clean it up.
> >
> > There might be similar issues in other drivers too.
>
> We can put all objects related to multifd load in their own struct (as 
> suggested by Cedric in patch #22) and allocate the struct only if multifd 
> device state transfer is used.
> Then in the cleanup flow we clean the struct only if it was allocated.
>
> This way we don't need to add the load_setup flag and we can keep the 
> SaveVMHandlers semantics as is.
>
> Do you think this will be OK?

I think here the discussion is more of whether we refactor the
{load,save}_cleanup handler semantics to "cleaner" design where
these handlers are only called if the relevant {load,save}_setup
handler was successfully called first (but at the same time risk
introducing regressions).

If we keep the existing semantics of these handlers (like this
patch set did) then it is just an implementation detail whether
we keep an explicit flag like "migration->load_setup" or have
a struct pointer that serves as an implicit equivalent flag
(when not NULL) - I don't have a strong opinion on this particular
detail.

> Thanks.

Thanks,
Maciej