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 <avih...@nvidia.com> >>>> The refactor itself is fine except a pre-existing bug: >>>> >>>> Reviewed-by: Joao Martins <joao.m.mart...@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-12d49e70f...@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-9bdc68168...@nvidia.com/