Re: [PATCH 2/9] vfio/migration: Refactor vfio_devices_all_dirty_tracking() logic

[Joao Martins]

On 16/12/2024 16:05, Joao Martins wrote:
> On 16/12/2024 15:52, Joao Martins wrote:
>> On 16/12/2024 14:52, Avihai Horon wrote:
>>>
>>> On 16/12/2024 14:32, Joao Martins wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 16/12/2024 09:46, Avihai Horon wrote:
>>>>> During dirty page log sync, vfio_devices_all_dirty_tracking() is used to
>>>>> check if dirty tracking has been started in order to avoid errors. The
>>>>> current logic checks if migration is in ACTIVE or DEVICE states to
>>>>> ensure dirty tracking has been started.
>>>>>
>>>>> However, recently there has been an effort to simplify the migration
>>>>> status API and reduce it to a single migration_is_running() function.
>>>>>
>>>>> To accommodate this, refactor vfio_devices_all_dirty_tracking() logic so
>>>>> it won't use migration_is_active() and migration_is_device(). Instead,
>>>>> use internal VFIO dirty tracking flags.
>>>>>
>>>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>>> The refactor itself is fine except a pre-existing bug:
>>>>
>>>>          Reviewed-by: Joao Martins <joao.m.martins@oracle.com>
>>>>
>>>>> ---
>>>>>   hw/vfio/common.c | 21 ++++++++++++++++++++-
>>>>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>>> index dcef44fe55..a99796403e 100644
>>>>> --- a/hw/vfio/common.c
>>>>> +++ b/hw/vfio/common.c
>>>>> @@ -170,11 +170,30 @@ bool vfio_device_state_is_precopy(VFIODevice 
>>>>> *vbasedev)
>>>>>              migration->device_state == VFIO_DEVICE_STATE_PRE_COPY_P2P;
>>>>>   }
>>>>>
>>>>> +static bool vfio_devices_all_device_dirty_tracking_started(
>>>>> +    const VFIOContainerBase *bcontainer)
>>>>> +{
>>>>> +    VFIODevice *vbasedev;
>>>>> +
>>>>> +    QLIST_FOREACH(vbasedev, &bcontainer->device_list, container_next) {
>>>>> +        if (!vbasedev->dirty_tracking) {
>>>>> +            return false;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return true;
>>>>> +}
>>>>> +
>>>>>   static bool vfio_devices_all_dirty_tracking(VFIOContainerBase 
>>>>> *bcontainer)
>>>>>   {
>>>>>       VFIODevice *vbasedev;
>>>>>
>>>>> -    if (!migration_is_active() && !migration_is_device()) {
>>>>> +    if (!migration_is_running()) {
>>>>> +        return false;
>>>>> +    }
>>>>> +
>>>> Tieing to migration status means that non-KVM dirty trackers cannot be 
>>>> toggled
>>>> unless somebody starts migration. When really your original intention 
>>>> behind
>>>> commit ff180c6bd7 ("vfio/migration: Skip log_sync during migration SETUP 
>>>> state")
>>>> was to avoid the setup state when you are indeed during a migration.
>>>
>>> It was tied to migration even prior to this commit, as VFIO log syncs were
>>> restricted to run only during migration (we had "if (!
>>> migration_is_setup_or_active())" check).
>>> This commit only narrowed it down further to not run during SETUP.
>>>
>>
>> Ok, good point.
>>
>> Btw you are regressing from that behaviour with this change above, because if
>> migration has state MIGRATION_STATUS_SETUP and migration_is_running() will
>> return true and so you will log dirty pages.
>>
> 
> Nevermind this comment.
> 
> Just noticed that it was the point of the whole thread:
> 
> https://lore.kernel.org/qemu-devel/78729b4b-3747-4408-8146-12d49e70fed1@nvidia.com/#r
> 
> ... where you discuss this.
> 

Actually, from the thread quoted in the cover letter[0], at the end of this
series we rely on status of dirty tracking to go ahead with the syncs instead of
relying on migration running or not.
Hence just removing migration_is_running() calls wouldn't regress the bug you
fixed with commit ff180c6bd7, neither calc-dirty-rate would miss dirty data from
its reports.

[0]
https://lore.kernel.org/qemu-devel/58146556-d3fa-4d8b-a1db-9bdc68168c78@nvidia.com/