Re: [PATCH v4 26/33] vfio/migration: Multifd device state transfer support - receive init/cleanup

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

On 2/14/25 21:55, Maciej S. Szmigiero wrote:
> On 12.02.2025 11:55, Cédric Le Goater wrote:
>> On 1/30/25 11:08, Maciej S. Szmigiero wrote:
>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>
>>> Add support for VFIOMultifd data structure that will contain most of the
>>> receive-side data together with its init/cleanup methods.
>>>
>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>> ---
>>>   hw/vfio/migration.c           | 52 +++++++++++++++++++++++++++++++++--
>>>   include/hw/vfio/vfio-common.h |  5 ++++
>>>   2 files changed, 55 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>> index 3211041939c6..bcdf204d5cf4 100644
>>> --- a/hw/vfio/migration.c
>>> +++ b/hw/vfio/migration.c
>>> @@ -300,6 +300,9 @@ typedef struct VFIOStateBuffer {
>>>       size_t len;
>>>   } VFIOStateBuffer;
>>> +typedef struct VFIOMultifd {
>>> +} VFIOMultifd;
>>> +
>>>   static void vfio_state_buffer_clear(gpointer data)
>>>   {
>>>       VFIOStateBuffer *lb = data;
>>> @@ -398,6 +401,18 @@ static int vfio_load_device_config_state(QEMUFile *f, void *opaque)
>>>       return qemu_file_get_error(f);
>>>   }
>>> +static VFIOMultifd *vfio_multifd_new(void)
>>> +{
>>> +    VFIOMultifd *multifd = g_new(VFIOMultifd, 1);
>>> +
>>> +    return multifd;
>>> +}
>>> +
>>> +static void vfio_multifd_free(VFIOMultifd *multifd)
>>> +{
>>> +    g_free(multifd);
>>> +}
>>> +
>>>   static void vfio_migration_cleanup(VFIODevice *vbasedev)
>>>   {
>>>       VFIOMigration *migration = vbasedev->migration;
>>> @@ -785,14 +800,47 @@ static void vfio_save_state(QEMUFile *f, void *opaque)
>>>   static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp)
>>>   {
>>>       VFIODevice *vbasedev = opaque;
>>> +    VFIOMigration *migration = vbasedev->migration;
>>> +    int ret;
>>> +
>>> +    /*
>>> +     * Make a copy of this setting at the start in case it is changed
>>> +     * mid-migration.
>>> +     */
>>> +    if (vbasedev->migration_multifd_transfer == ON_OFF_AUTO_AUTO) {
>>> +        migration->multifd_transfer = vfio_multifd_transfer_supported();
>>
>> Attribute "migration->multifd_transfer" is not necessary. It can be
>> replaced by a small inline helper testing pointer migration->multifd
>> and this routine can use a local variable instead.
> 
> It's necessary for the send side since it does not need/allocate VFIOMultifd
> at migration->multifd, so this (receive) side can use it for commonality too.

Hmm, we can allocate migration->multifd on the send side too, even
if the attributes are unused and it is up to vfio_multifd_free() to
make the difference between the send/recv side.


Something that is bothering me is the lack of introspection tools
and statistics. What could be possibly added under VFIOMultifd and
VfioStats ?

>> I don't think the '_transfer' suffix adds much to the understanding.
> 
> The migration->multifd was already taken by VFIOMultifd struct, but
> it could use other name (migration->multifd_switch? migration->multifd_on?).

yeah. Let's try to get rid of it first.
  
>>> +    } else {
>>> +        migration->multifd_transfer =
>>> +            vbasedev->migration_multifd_transfer == ON_OFF_AUTO_ON;
>>> +    }
>>> +
>>> +    if (migration->multifd_transfer && !vfio_multifd_transfer_supported()) {
>>> +        error_setg(errp,
>>> +                   "%s: Multifd device transfer requested but unsupported in the current config",
>>> +                   vbasedev->name);
>>> +        return -EINVAL;
>>> +    }
>>
>> The above checks are also introduced in vfio_save_setup(). Please
>> implement a common routine vfio_multifd_is_enabled() or some other
>> name.
> 
> Done (as common vfio_multifd_transfer_setup()).

vfio_multifd_is_enabled() please, returning a bool.

> 
>>> +    ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
>>> +                                   migration->device_state, errp);
>>> +    if (ret) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (migration->multifd_transfer) {
>>> +        assert(!migration->multifd);
>>> +        migration->multifd = vfio_multifd_new();
>>> +    }
>>> -    return vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_RESUMING,
>>> -                                    vbasedev->migration->device_state, errp);
>>> +    return 0;
>>>   }
>>>   static int vfio_load_cleanup(void *opaque)
>>>   {
>>>       VFIODevice *vbasedev = opaque;
>>> +    VFIOMigration *migration = vbasedev->migration;
>>> +
>>> +    g_clear_pointer(&migration->multifd, vfio_multifd_free);
>>
>> please add a vfio_multifd_cleanup() routine.
>>
> 
> Done.
> 
>>>       vfio_migration_cleanup(vbasedev);
>>>       trace_vfio_load_cleanup(vbasedev->name);
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>> index 153d03745dc7..c0c9c0b1b263 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -61,6 +61,8 @@ typedef struct VFIORegion {
>>>       uint8_t nr; /* cache the region number for debug */
>>>   } VFIORegion;
>>> +typedef struct VFIOMultifd VFIOMultifd;
>>> +
>>>   typedef struct VFIOMigration {
>>>       struct VFIODevice *vbasedev;
>>>       VMChangeStateEntry *vm_state;
>>> @@ -72,6 +74,8 @@ typedef struct VFIOMigration {
>>>       uint64_t mig_flags;
>>>       uint64_t precopy_init_size;
>>>       uint64_t precopy_dirty_size;
>>> +    bool multifd_transfer;
>>> +    VFIOMultifd *multifd;
>>>       bool initial_data_sent;
>>>       bool event_save_iterate_started;
>>> @@ -133,6 +137,7 @@ typedef struct VFIODevice {
>>>       bool no_mmap;
>>>       bool ram_block_discard_allowed;
>>>       OnOffAuto enable_migration;
>>> +    OnOffAuto migration_multifd_transfer;
>>
>> This property should be added at the end of the series, with documentation,
>> and used in the vfio_multifd_some_name() routine I mentioned above.
>>
> 
> The property behind this variable *is* in fact introduced at the end of the series -
> in a commit called "vfio/migration: Add x-migration-multifd-transfer VFIO property"
> after which there are only commits adding the related compat entry and a VFIO
> developer doc update.
> 
> The variable itself needs to be introduced earlier since various newly
> introduced code blocks depend on its value to only get activated when multifd
> transfer is enabled.

Not if you introduce a vfio_multifd_is_enabled() routine hiding
the details. In that case, the property and attribute can be added
at the end of the series and you don't need to add the attribute
earlier.


Thanks,

C.