Re: [PATCH 1/4] vfio/migration: Add save_{iterate,complete_precopy}_started trace events

[Avihai Horon <avihaih@nvidia.com>]

On 04/11/2024 16:00, Maciej S. Szmigiero wrote:
> External email: Use caution opening links or attachments
>
>
> On 4.11.2024 09:08, Avihai Horon wrote:
>>
>> On 01/11/2024 0:17, Maciej S. Szmigiero wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> Hi Avihai,
>>>
>>> On 31.10.2024 15:21, Avihai Horon wrote:
>>>> Hi Maciej,
>>>>
>>>> On 29/10/2024 16:58, Maciej S. Szmigiero wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>>>
>>>>> This way both the start and end points of migrating a particular VFIO
>>>>> device are known.
>>>>>
>>>>> Add also a vfio_save_iterate_empty_hit trace event so it is known 
>>>>> when
>>>>> there's no more data to send for that device.
>>>>>
>>>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>>> ---
>>>>>   hw/vfio/migration.c           | 13 +++++++++++++
>>>>>   hw/vfio/trace-events          |  3 +++
>>>>>   include/hw/vfio/vfio-common.h |  3 +++
>>>>>   3 files changed, 19 insertions(+)
>>>>>
>>>>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>>>>> index 992dc3b10257..1b1ddf527d69 100644
>>>>> --- a/hw/vfio/migration.c
>>>>> +++ b/hw/vfio/migration.c
>>>>> @@ -472,6 +472,9 @@ static int vfio_save_setup(QEMUFile *f, void 
>>>>> *opaque, Error **errp)
>>>>>           return -ENOMEM;
>>>>>       }
>>>>>
>>>>> +    migration->save_iterate_started = false;
>>>>> +    migration->save_iterate_empty_hit = false;
>>>>> +
>>>>>       if (vfio_precopy_supported(vbasedev)) {
>>>>>           switch (migration->device_state) {
>>>>>           case VFIO_DEVICE_STATE_RUNNING:
>>>>> @@ -602,9 +605,17 @@ static int vfio_save_iterate(QEMUFile *f, 
>>>>> void *opaque)
>>>>>       VFIOMigration *migration = vbasedev->migration;
>>>>>       ssize_t data_size;
>>>>>
>>>>> +    if (!migration->save_iterate_started) {
>>>>> + trace_vfio_save_iterate_started(vbasedev->name);
>>>>> +        migration->save_iterate_started = true;
>>>>> +    }
>>>>> +
>>>>>       data_size = vfio_save_block(f, migration);
>>>>>       if (data_size < 0) {
>>>>>           return data_size;
>>>>> +    } else if (data_size == 0 && 
>>>>> !migration->save_iterate_empty_hit) {
>>>>> + trace_vfio_save_iterate_empty_hit(vbasedev->name);
>>>>> +        migration->save_iterate_empty_hit = true;
>>>>>       }
>>>>
>>>> Can we instead use trace_vfio_save_iterate to understand if the 
>>>> device reached 0?
>>>
>>> AFAIK there's not way to filter trace events by their parameters,
>>> like only logging vfio_save_iterate trace event if both parameters
>>> are zero.
>>>
>>> It means that vfio_save_iterate has to be enabled unconditionally to
>>> serve as a replacement for vfio_save_iterate_empty_hit, which could
>>> result in it being logged/emitted many extra times (with non-zero
>>> parameters).
>>>
>>> Because of that I think having a dedicated trace event for such
>>> occasion makes sense (it is also easily grep-able).
>>
>> Ahh, I understand.
>>
>>>
>>>> In any case, I think the above could fit better in 
>>>> vfio_save_block(), where ENOMSG indicates that the device has no 
>>>> more data to send during pre-copy phase:
>>>>
>>>> ...
>>>> if (data_size < 0) {
>>>>      /*
>>>>       * Pre-copy emptied all the device state for now. For more 
>>>> information,
>>>>       * please refer to the Linux kernel VFIO uAPI.
>>>>       */
>>>>      if (errno == ENOMSG) {
>>>> trace_vfio_save_iterate_empty_hit(vbasedev->name) <--------------- 
>>>> move it here
>>>>          return 0;
>>>>      }
>>>>
>>>>      return -errno;
>>>> }
>>>> ...
>>>>
>>>> If you move the trace there, maybe renaming it to 
>>>> trace_vfio_precopy_empty_hit() will be more accurate?
>>>
>>> This move and rename seems sensible to me.
>>>
>>>> And trying to avoid adding the extra 
>>>> VFIOMigration->save_iterate_empty_hit flag, can we simply trace it 
>>>> every time?
>>>
>>> Will have to do some tests to be sure but if there's possibility that
>>> we get ENOMSG many times then obviously we don't want to flood logs 
>>> with
>>> this trace event in this case - we want to only log the
>>> "data present" -> "data not present" edge/change.
>>
>> OK, so I guess a flag is really needed.
>> BTW, there is also trace_vfio_state_pending_exact, maybe it could do 
>> the job? It might get called multiple times but not as many as 
>> vfio_save_iterate.
>
> In a quick test run it was still called/logged 5 times for each VFIO 
> device
> so quite more often than the empty_hit one (which was logged just once 
> per dev).

Yes, that is expected.

If that's too noisy for you then the empty_hit trace seems fine, IMHO.

Thanks.