Re: [PATCH v6 25/36] vfio/migration: Setup and cleanup multifd transfer in these general methods

["Cédric Le Goater" <clg@redhat.com>]

On 3/5/25 17:39, 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.
  
I think this is close to the initial proposal of Maciej in v5 and
I asked to do it that way in v6, moslty because we don't agree on
the need of 'bool multifd_transfer'.

Since it's minor, we can refactor afterwards. For now, let's keep
it as it is please.

Thanks,

C.