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

[Avihai Horon]

On 16/12/2024 21:53, Joao Martins wrote:
> External email: Use caution opening links or attachments
>
>
> 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.

Right.
But since it was previously tied to migration and because I remembered 
you had the calc-dirty-rate series that also added the -d param to 
control the scope of dirty rate, I thought I should only stick to 
refactoring for the migration status API simplification and that the 
calc-dirty-rate series would come after.

But if we simply want to include VFIO DPT in calc-dirty-rate, then yes, 
I can untie it from migration in v2.

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