Re: [PATCH v5 26/36] vfio/migration: Multifd device state transfer support - received buffers queuing

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

On 2/26/25 22:04, Maciej S. Szmigiero wrote:
> On 26.02.2025 11:43, Cédric Le Goater wrote:
>> On 2/19/25 21:34, Maciej S. Szmigiero wrote:
>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>
>>> The multifd received data needs to be reassembled since device state
>>> packets sent via different multifd channels can arrive out-of-order.
>>>
>>> Therefore, each VFIO device state packet carries a header indicating its
>>> position in the stream.
>>> The raw device state data is saved into a VFIOStateBuffer for later
>>> in-order loading into the device.
>>>
>>> The last such VFIO device state packet should have
>>> VFIO_DEVICE_STATE_CONFIG_STATE flag set and carry the device config state.
>>>
>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>> ---
>>>   hw/vfio/migration-multifd.c | 103 ++++++++++++++++++++++++++++++++++++
>>>   hw/vfio/migration-multifd.h |   3 ++
>>>   hw/vfio/migration.c         |   1 +
>>>   hw/vfio/trace-events        |   1 +
>>>   4 files changed, 108 insertions(+)
>>>
>>> diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
>>> index c2defc0efef0..5d5ee1393674 100644
>>> --- a/hw/vfio/migration-multifd.c
>>> +++ b/hw/vfio/migration-multifd.c
>>> @@ -42,6 +42,11 @@ typedef struct VFIOStateBuffer {
>>>   } VFIOStateBuffer;
>>>   typedef struct VFIOMultifd {
>>> +    VFIOStateBuffers load_bufs;
>>> +    QemuCond load_bufs_buffer_ready_cond;
>>> +    QemuMutex load_bufs_mutex; /* Lock order: this lock -> BQL */
>>> +    uint32_t load_buf_idx;
>>> +    uint32_t load_buf_idx_last;
>>>   } VFIOMultifd;
>>>   static void vfio_state_buffer_clear(gpointer data)
>>> @@ -87,15 +92,113 @@ static VFIOStateBuffer *vfio_state_buffers_at(VFIOStateBuffers *bufs, guint idx)
>>>       return &g_array_index(bufs->array, VFIOStateBuffer, idx);
>>>   }
>>
>> this routine expects load_bufs_mutex to be locked ? May be say so.
> 
> I guess the comment above pertains to the vfio_load_state_buffer_insert()
> below.
> 
> Do you mean it should have a comment that it expects to be called
> under load_bufs_mutex?

Just a one liner like :

/* called with load_bufs_mutex locked */

?

It's good to have for the future generations.

> 
>>> +static bool vfio_load_state_buffer_insert(VFIODevice *vbasedev,
>>
>> could you pass VFIOMultifd* instead  ?
> 
> No, it needs vbasedev->migration_max_queued_buffers too (introduced
> in later patch).
> > Also, most of VFIO routines (besides very small helpers/wrappers)
> take VFIODevice *.

OK. It's minor but I prefer when parameters are limited to the minimum.
Having 'VFIODevice *' opens doors to a lot of state.


Thanks,

C.



> 
>>> +                                          VFIODeviceStatePacket *packet,
>>> +                                          size_t packet_total_size,
>>> +                                          Error **errp)
>>> +{
>>> +    VFIOMigration *migration = vbasedev->migration;
>>> +    VFIOMultifd *multifd = migration->multifd;
>>> +    VFIOStateBuffer *lb;
>>> +
>>> +    vfio_state_buffers_assert_init(&multifd->load_bufs);
>>> +    if (packet->idx >= vfio_state_buffers_size_get(&multifd->load_bufs)) {
>>> +        vfio_state_buffers_size_set(&multifd->load_bufs, packet->idx + 1);
>>> +    }
>>> +
>>> +    lb = vfio_state_buffers_at(&multifd->load_bufs, packet->idx);
>>> +    if (lb->is_present) {
>>> +        error_setg(errp, "state buffer %" PRIu32 " already filled",
>>> +                   packet->idx);
>>> +        return false;
>>> +    }
>>> +
>>> +    assert(packet->idx >= multifd->load_buf_idx);
>>> +
>>> +    lb->data = g_memdup2(&packet->data, packet_total_size - sizeof(*packet));
>>> +    lb->len = packet_total_size - sizeof(*packet);
>>> +    lb->is_present = true;
>>> +
>>> +    return true;
>>> +}
>>> +
>>> +bool vfio_load_state_buffer(void *opaque, char *data, size_t data_size,
>>> +                            Error **errp)
>>
>>
>> AFAICS, the only users of the .load_state_buffer() handlers is
>> multifd_device_state_recv().
>>
>> Please rename to vfio_multifd_load_state_buffer().
> 
> Renamed it accordingly.
> 
>>> +{
>>> +    VFIODevice *vbasedev = opaque;
>>> +    VFIOMigration *migration = vbasedev->migration;
>>> +    VFIOMultifd *multifd = migration->multifd;
>>> +    VFIODeviceStatePacket *packet = (VFIODeviceStatePacket *)data;
>>> +
>>> +    /*
>>> +     * Holding BQL here would violate the lock order and can cause
>>> +     * a deadlock once we attempt to lock load_bufs_mutex below.
>>> +     */
>>> +    assert(!bql_locked());
>>> +
>>> +    if (!vfio_multifd_transfer_enabled(vbasedev)) {
>>> +        error_setg(errp,
>>> +                   "got device state packet but not doing multifd transfer");
>>> +        return false;
>>> +    }
>>> +
>>> +    assert(multifd);
>>> +
>>> +    if (data_size < sizeof(*packet)) {
>>> +        error_setg(errp, "packet too short at %zu (min is %zu)",
>>> +                   data_size, sizeof(*packet));
>>> +        return false;
>>> +    }
>>> +
>>> +    if (packet->version != VFIO_DEVICE_STATE_PACKET_VER_CURRENT) {
>>> +        error_setg(errp, "packet has unknown version %" PRIu32,
>>> +                   packet->version);
>>> +        return false;
>>> +    }
>>> +
>>> +    if (packet->idx == UINT32_MAX) {
>>> +        error_setg(errp, "packet has too high idx");
>>
>> or "packet index is invalid" ?
> 
> Changed the error message.
> 
>>> +        return false;
>>> +    }
>>> +
>>> +    trace_vfio_load_state_device_buffer_incoming(vbasedev->name, packet->idx);
>>> +
>>> +    QEMU_LOCK_GUARD(&multifd->load_bufs_mutex);
>>
>> Using WITH_QEMU_LOCK_GUARD() would be cleaner I think.
> 
> Changed into a WITH_QEMU_LOCK_GUARD() block.
> 
>>
>>
>> Thanks,
>>
>> C.
> 
> Thanks,
> Maciej
>