Re: [PATCH 1/2] vfio/migration: Add also max in-flight VFIO device state buffers size limit

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

On 3/7/25 14:45, Maciej S. Szmigiero wrote:
> On 7.03.2025 13:03, Cédric Le Goater wrote:
>> On 3/7/25 11:57, Maciej S. Szmigiero wrote:
>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>
>>> There's already a max in-flight VFIO device state buffers *count* limit,
>>
>> no. there isn't. Do we need both ?
> 
> This is on a top of the remaining patches (x-migration-load-config-after-iter
> and x-migration-max-queued-buffers) - I thought we were supposed to work
> on these after the main series was merged as they are relatively non-critical.

yes. we don't need both count and size limits though, a size limit is enough.

> I would also give x-migration-load-config-after-iter priority over
> x-migration-max-queued-buffers{,-size} as the former is correctness fix
> while the later are just additional functionalities.

ok. I have kept both patches in my tree with the doc updates.

> Also, if some setup is truly worried about these buffers consuming too much
> memory then roughly the same thing could be achieved by (temporarily) putting
> the target QEMU process in a memory-limited cgroup.

yes.

That said,

since QEMU exchanges 1MB VFIODeviceStatePackets when using multifd and that
the overall device state is in the order of 100MB :

   /*
    * This is an arbitrary size based on migration of mlx5 devices, where typically
    * total device migration size is on the order of 100s of MB. Testing with
    * larger values, e.g. 128MB and 1GB, did not show a performance improvement.
    */
   #define VFIO_MIG_DEFAULT_DATA_BUFFER_SIZE (1 * MiB)


Could we define the limit to 1GB ?

Avihai, would that make sense  ?


Thanks,

C.




> 
> On the other hand, the network endianess patch is urgent since it affects
> the bit stream.
> 
>>> add also max queued buffers *size* limit.
>>>
>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>> ---
>>>   docs/devel/migration/vfio.rst |  8 +++++---
>>>   hw/vfio/migration-multifd.c   | 21 +++++++++++++++++++--
>>>   hw/vfio/pci.c                 |  9 +++++++++
>>>   include/hw/vfio/vfio-common.h |  1 +
>>>   4 files changed, 34 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/docs/devel/migration/vfio.rst b/docs/devel/migration/vfio.rst
>>> index 7c9cb7bdbf87..127a1db35949 100644
>>> --- a/docs/devel/migration/vfio.rst
>>> +++ b/docs/devel/migration/vfio.rst
>>> @@ -254,12 +254,14 @@ This means that a malicious QEMU source could theoretically cause the target
>>>   QEMU to allocate unlimited amounts of memory for such buffers-in-flight.
>>>   The "x-migration-max-queued-buffers" property allows capping the maximum count
>>> -of these VFIO device state buffers queued at the destination.
>>> +of these VFIO device state buffers queued at the destination while
>>> +"x-migration-max-queued-buffers-size" property allows capping their total queued
>>> +size.
>>>   Because a malicious QEMU source causing OOM on the target is not expected to be
>>>   a realistic threat in most of VFIO live migration use cases and the right value
>>> -depends on the particular setup by default this queued buffers limit is
>>> -disabled by setting it to UINT64_MAX.
>>> +depends on the particular setup by default these queued buffers limits are
>>> +disabled by setting them to UINT64_MAX.
>>>   Some host platforms (like ARM64) require that VFIO device config is loaded only
>>>   after all iterables were loaded.
>>> diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
>>> index dccd763d7c39..a9d41b9f1cb1 100644
>>> --- a/hw/vfio/migration-multifd.c
>>> +++ b/hw/vfio/migration-multifd.c
>>> @@ -83,6 +83,7 @@ typedef struct VFIOMultifd {
>>>       uint32_t load_buf_idx;
>>>       uint32_t load_buf_idx_last;
>>>       uint32_t load_buf_queued_pending_buffers;
>>
>> 'load_buf_queued_pending_buffers' is not in mainline. Please rebase.
>>
>>
>> Thanks,
>>
>> C.
> 
> Thanks,
> Maciej
>