Re: [PATCH v2 17/17] vfio/migration: Multifd device state transfer support - send side

[Avihai Horon <avihaih@nvidia.com>]

On 09/09/2024 21:07, Maciej S. Szmigiero wrote:
> External email: Use caution opening links or attachments
>
>
> On 9.09.2024 13:41, Avihai Horon wrote:
>>
>> On 27/08/2024 20:54, Maciej S. Szmigiero wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>
>>> Implement the multifd device state transfer via additional per-device
>>> thread inside save_live_complete_precopy_thread handler.
>>>
>>> Switch between doing the data transfer in the new handler and doing it
>>> in the old save_state handler depending on the
>>> x-migration-multifd-transfer device property value.
>>>
>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>> ---
>>>   hw/vfio/migration.c           | 169 
>>> ++++++++++++++++++++++++++++++++++
>>>   hw/vfio/trace-events          |   2 +
>>>   include/hw/vfio/vfio-common.h |   1 +
>>>   3 files changed, 172 insertions(+)
>>>
>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>> index 57c1542528dc..67996aa2df8b 100644
>>> --- a/hw/vfio/migration.c
>>> +++ b/hw/vfio/migration.c
>>> @@ -655,6 +655,16 @@ static int vfio_save_setup(QEMUFile *f, void 
>>> *opaque, Error **errp)
>>>       uint64_t stop_copy_size = VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE;
>>>       int ret;
>>>
>>> +    /* Make a copy of this setting at the start in case it is 
>>> changed mid-migration */
>>> +    migration->multifd_transfer = 
>>> vbasedev->migration_multifd_transfer;
>>
>> Should VFIO multifd be controlled by main migration multifd 
>> capability, and let the per VFIO device migration_multifd_transfer 
>> property be immutable and enabled by default?
>> Then we would have a single point of configuration (and an extra one 
>> per VFIO device just to disable for backward compatibility).
>> Unless there are other benefits to have this property configurable?
>
> We want multifd device state transfer property to be configurable 
> per-device
> in case in the future we add another device type (besides VFIO) that 
> supports
> multifd device state transfer.
>
> In this case, we might need to enable the multifd device state 
> transfer just
> for VFIO devices, but not for this new device type when we are 
> migrating to a
> QEMU target that supports just the VFIO multifd device state transfer.

I think for this case we can use hw/core/machine.c:hw_compat_X_Y arrays [1].

[1] 
https://www.qemu.org/docs/master/devel/migration/compatibility.html#how-backwards-compatibility-works

>
> TBH, I'm not opposed to adding a additional global multifd device 
> state transfer
> switch (if we keep the per-device ones too) but I am not sure what 
> value it adds.
>
>>> +
>>> +    if (migration->multifd_transfer && 
>>> !migration_has_device_state_support()) {
>>> +        error_setg(errp,
>>> +                   "%s: Multifd device transfer requested but 
>>> unsupported in the current config",
>>> +                   vbasedev->name);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>>       qemu_put_be64(f, VFIO_MIG_FLAG_DEV_SETUP_STATE);
>>>
>>>       vfio_query_stop_copy_size(vbasedev, &stop_copy_size);
>>> @@ -835,10 +845,20 @@ static int vfio_save_iterate(QEMUFile *f, void 
>>> *opaque)
>>>   static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>>>   {
>>>       VFIODevice *vbasedev = opaque;
>>> +    VFIOMigration *migration = vbasedev->migration;
>>>       ssize_t data_size;
>>>       int ret;
>>>       Error *local_err = NULL;
>>>
>>> +    if (migration->multifd_transfer) {
>>> +        /*
>>> +         * Emit dummy NOP data, vfio_save_complete_precopy_thread()
>>> +         * does the actual transfer.
>>> +         */
>>> +        qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>>
>> There are three places where we send this dummy end of state, maybe 
>> worth extracting it to a helper? I.e., vfio_send_end_of_state() and 
>> then document there the rationale.
>
> I'm not totally against it but it's wrapping just a single line of 
> code in
> a separate function?

Yes, it's more for self-documentation purpose and for not duplicating 
comments.
I guess it's a matter of taste, so we can go either way and let 
maintainer decide.

>
>>> +        return 0;
>>> +    }
>>> +
>>> trace_vfio_save_complete_precopy_started(vbasedev->name);
>>>
>>>       /* We reach here with device state STOP or STOP_COPY only */
>>> @@ -864,12 +884,159 @@ static int 
>>> vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>>>       return ret;
>>>   }
>>>
>>> +static int 
>>> vfio_save_complete_precopy_async_thread_config_state(VFIODevice 
>>> *vbasedev,
>>> +                                                                
>>> char *idstr,
>>> + uint32_t instance_id,
>>> + uint32_t idx)
>>> +{
>>> +    g_autoptr(QIOChannelBuffer) bioc = NULL;
>>> +    QEMUFile *f = NULL;
>>> +    int ret;
>>> +    g_autofree VFIODeviceStatePacket *packet = NULL;
>>> +    size_t packet_len;
>>> +
>>> +    bioc = qio_channel_buffer_new(0);
>>> +    qio_channel_set_name(QIO_CHANNEL(bioc), 
>>> "vfio-device-config-save");
>>> +
>>> +    f = qemu_file_new_output(QIO_CHANNEL(bioc));
>>> +
>>> +    ret = vfio_save_device_config_state(f, vbasedev, NULL);
>>> +    if (ret) {
>>> +        return ret;
>>
>> Need to close f in this case.
>
> Right - by the way, that's a good example why RAII
> helps avoid such mistakes.

Agreed :)

>
>>> +    }
>>> +
>>> +    ret = qemu_fflush(f);
>>> +    if (ret) {
>>> +        goto ret_close_file;
>>> +    }
>>> +
>>> +    packet_len = sizeof(*packet) + bioc->usage;
>>> +    packet = g_malloc0(packet_len);
>>> +    packet->idx = idx;
>>> +    packet->flags = VFIO_DEVICE_STATE_CONFIG_STATE;
>>> +    memcpy(&packet->data, bioc->data, bioc->usage);
>>> +
>>> +    if (!multifd_queue_device_state(idstr, instance_id,
>>> +                                    (char *)packet, packet_len)) {
>>> +        ret = -1;
>>
>> goto ret_close_file?
>
> Right, it would be better not to increment the counter in this case.
>
>>> +    }
>>> +
>>> +    bytes_transferred += packet_len;
>>
>> bytes_transferred is a global variable. Now that we access it from 
>> multiple threads it should be protected.
>
> Right, this stat needs some concurrent access protection.
>
>> Note that now the VFIO device data is reported also in multifd stats 
>> (if I am not mistaken), is this the behavior we want? Maybe we should 
>> enhance multifd stats to distinguish between RAM data and device data?
>
> Multifd stats report total size of data transferred via multifd so
> they should include device state too.

Yes I agree. But now we are reporting double the amount of VFIO data 
that we actually transfer (once in "vfio device transferred" and another 
in multifd stats) and this may be misleading.
So maybe we should add a dedicated multifd device state counter and 
report VFIO multifd bytes there instead of in bytes_transferred?
We can wait for other people's opinion about that.

>
> It may make sense to add a dedicated device state transfer counter
> at some time though.
>
>>> +
>>> +ret_close_file:
>>
>> Rename to "out" as we only have one exit point?
>>
>>> +    g_clear_pointer(&f, qemu_fclose);
>>
>> f is a local variable, wouldn't qemu_fclose(f) be enough here?
>
> Sure, but why leave a dangling pointer?
>
> Currently, it is obviously a NOP (probably deleted by dead store
> elimination anyway) but the code might get refactored at some point
> and I think it's good practice to always NULL pointers after freeing
> them where possible and so be on the safe side.

Ack.

Thanks.