On 11/12/2024 1:04, Maciej S. Szmigiero wrote:
External email: Use caution opening links or attachments
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).
Yes, and I agree with you that changing the semantics of SaveVMHandlers can be
risky and may deserve a series of its own.
But Cedric didn't like the flag option, so I suggested to do what we usually
do, AFAIU, which is to check if the structs are allocated and need cleanup.
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.
I prefer the struct pointer way, it seems less cumbersome to me.
But it's Cedric's call at the end.