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

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

On 2/17/25 23:13, Maciej S. Szmigiero wrote:
> On 17.02.2025 10:38, Cédric Le Goater wrote:
>> 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.
> 
> Allocating an unnecessary VFIOMultifd structure that has 12 members,
> some of them complex like QemuThread, QemuCond or QemuMutex, just
> to avoid having one extra bool variable (migration_multifd_transfer or
> whatever it ends being named) seem like a poor trade-off for me.
> 
>>
>> Something that is bothering me is the lack of introspection tools
>> and statistics. What could be possibly added under VFIOMultifd and
>> VfioStats ?
> 
> There's already VFIO bytes transferred counter and also a
> multifd bytes transferred counter.
> 
> There are quite a few trace events (both existing and newly added
> by this patch).
> 
> While even more statistics and traces may help with tuning/debugging
> in some cases that's something easily added in the future.
> 
>>>> 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.
> 
> Functions named *_is_something() normally just check some conditions
> and return a computed value without having any side effects.
> 
> Here, vfio_multifd_transfer_setup() also sets migration->multifd_transfer
> appropriately (or could migration->multifd) - that's common code for
> save and load.
> 
> I guess you meant to move something else rather than this block
> of code into vfio_multifd_is_enabled() - see my answer below.
> 
>>>>>       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.
> 
> The part above that you wanted to be moved into vfio_multifd_is_enabled()
> is one-time check for load or save setup time.
> 
> That's *not* the switch to be tested by other parts of the code
> during the migration process to determine whether multifd transfer
> is in use.
> 
> If you want vfio_multifd_is_enabled() to be that switch that's tested by
> other parts of the VFIO migration code then it will finally consist of
> just a single line of code:
> "return migration->multifd_transfer" (or "return migration->multifd").
> 
> Then indeed the variable could be introduced with the property than
> controls it, but a dummy vfio_multifd_is_enabled() will need to be
> introduced earlier as "return false" to not break the build.


Sorry but I have switched to another series now, the one for live update,
and I will recheck your v5 proposal.


Thanks,

C.