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/11/25 17:01, Avihai Horon wrote:
> 
> On 11/03/2025 17:45, Cédric Le Goater wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 3/11/25 15:57, Avihai Horon wrote:
>>>
>>> On 11/03/2025 15:04, Cédric Le Goater wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 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  ?
>>>>
>>> There can be many use cases, each one with its own requirements and constraints, so it's hard for me to think of a "good" default value.
>>>
>>> IIUC this limit is mostly relevant for the extreme cases where devices have big state + writing the buffers to the device is slow.
>>> So IMHO let's set it to unlimited by default and let the users decide if they want to set such limit and to what value. (Note also that even when unlimited, it is really limited to 2 * device_state_size).
>>>
>>> Unless you have other reasons why 1GB or other value is preferable?
>>
>> none but UINT_MAX is not good value either. 
> 
> You mean UINT_MAX is not a good value to represent "unlimited" or that unlimited is not a good default value?

unlimited is not a good default value.

Thanks,

C.