On Wed, Mar 05, 2025 at 05:27:19PM +0100, Maciej S. Szmigiero wrote:
> On 5.03.2025 17:22, Peter Xu wrote:
> > On Tue, Mar 04, 2025 at 11:03:52PM +0100, Maciej S. Szmigiero wrote:
> > > @@ -509,6 +513,9 @@ static void vfio_save_cleanup(void *opaque)
> > > Error *local_err = NULL;
> > > int ret;
> > > + /* Currently a NOP, done for symmetry with load_cleanup() */
> > > + vfio_multifd_cleanup(vbasedev);
> >
> > So I just notice this when looking at the cleanup path. It can be super
> > confusing to cleanup the load threads in save().. IIUC we should drop it.
> >
>
> It's a NOP since in the save operation migration->multifd is going to be
> NULL so that "g_clear_pointer(&migration->multifd, vfio_multifd_free)"
> inside it won't do anything.
>
> Cedric suggested calling it anyway since vfio_save_setup() calls
> vfio_multifd_setup() so to be consistent we should call
> vfio_multifd_cleanup() on cleanup too.
>
> I think calling it makes sense since otherwise that vfio_multifd_setup()
> calls looks unbalanced.
IMHO we should split vfio_multifd_setup() into two functions:
- vfio_multifd_supported(): covering the first half of the fn, detect
whether it's supported all over and return the result.
- vfio_load_setup_multifd(): covering almost only vfio_multifd_new().
Then:
- the 1st function should be used in both save_setup() and
load_setup(). Meanwhile vfio_load_setup_multifd() should only be
invoked in load_setup().
- we rename vfio_multifd_cleanup() to vfio_multifd_load_cleanup(),
because that's really only about load..
- vfio_multifd_setup() (or after it renamed..) can drop the redundant
alloc_multifd parameter.
--
Peter Xu
