On Wed, Mar 05, 2025 at 11:39:23AM -0500, Peter Xu wrote: > 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.
PS: I'm OK if you and Cedric prefer having this discussed after merging the initial version, e.g. during hard-freeze. It would still be good to clean it up when at it though, if you agree. -- Peter Xu
