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

[Avihai Horon]

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.

> Now you can actually start/sync/stop dirty trackers without migration when you
> use calc-dirty-rate which is immensely useful to draw out how active a VM prior
> to starting migration.
>
> The fix is simple and would be to flex the condition to be something like:
>
>          /* Migration status is 'none' with calc-dirty-rate */
>          if (!migration_is_none() && !migration_is_running()) {
>              return false;
>          }
>
> This is ortoghonal to your series of course, but given you are skimming around
> this area, sounded like a good idea to raise this. This patch below is what I
> had plan to send when the development window started, but this was before folks
> wanted to unexport migration status helpers.

I remember you had several patches that formally added VFIO DPT to 
calc-dirty-rate (with a new "-d" QMP parameter and everything).
Are you still planning to send these?

> What would be the alternative idea
> forward?

Now we have an internal VFIO flag to indicate dirty tracking status, so 
that's one thing we can rely on.
And we can also use the global dirty tracking flags in 
include/exec/memory.h:

    /* Possible bits for global_dirty_log_{start|stop} */

    /* Dirty tracking enabled because migration is running */
    #define GLOBAL_DIRTY_MIGRATION  (1U << 0)

    /* Dirty tracking enabled because measuring dirty rate */
    #define GLOBAL_DIRTY_DIRTY_RATE (1U << 1)

    /* Dirty tracking enabled because dirty limit */
    #define GLOBAL_DIRTY_LIMIT      (1U << 2)

    #define GLOBAL_DIRTY_MASK  (0x7)

    extern unsigned int global_dirty_tracking;

So I guess we can add some helpers to access global_dirty_tracking and 
use them in VFIO to decide when to allow log sync.

But as you wrote, I think that's orthogonal to this series.

Thanks.

> -------------------->8---------------------
>
>  From ace22f29a0547353e4ed5a0db53292a77f79fa81 Mon Sep 17 00:00:00 2001
> From: Joao Martins <joao.m.martins@oracle.com>
> Date: Wed, 9 Oct 2024 00:27:46 +0100
> Subject: [PATCH] vfio/migration: Allow dirty tracking reports with
>   MIGRATION_STATUS_NONE
>
> Invoking calc-dirty-rate HMP/QMP method queries the VM dirty rate
> without starting a live migration, which is useful e.g. to understand how
> active guests are and even for testing purposes. calc-dirty-rate asks
> the dirty rate from the VM and it's not restricted to a particular dirty
> tracker.
>
> However commit ff180c6bd7 ("vfio/migration: Skip log_sync during migration SETUP
> state")
> didn't consider this and currently restricts that VF/IOMMU dirty info when 
> migration
> is active to allow it to be skipped during SETUP stage.
>
> The vfio dirty tracker is already started, the reports are just skipped
> based on migration status. So change vfio_devices_all_dirty_tracking() such
> that we include MIGRATION_STATUS_NONE to cover calc-dirty-rate case.
>
> Fixes: ff180c6bd7 ("vfio/migration: Skip log_sync during migration SETUP state")
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>   hw/vfio/common.c         | 4 +++-
>   include/migration/misc.h | 1 +
>   migration/migration.c    | 7 +++++++
>   3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index dcef44fe55be..0c188a2baac2 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -174,7 +174,9 @@ static bool
> vfio_devices_all_dirty_tracking(VFIOContainerBase *bcontainer)
>   {
>       VFIODevice *vbasedev;
>
> -    if (!migration_is_active() && !migration_is_device()) {
> +    /* Migration status is 'none' with calc-dirty-rate */
> +    if (!migration_is_none() &&
> +        !migration_is_active() && !migration_is_device()) {
>           return false;
>       }
>
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index 804eb23c0607..857768b51383 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -53,6 +53,7 @@ void dump_vmstate_json_to_file(FILE *out_fp);
>   void migration_object_init(void);
>   void migration_shutdown(void);
>
> +bool migration_is_none(void);
>   bool migration_is_active(void);
>   bool migration_is_device(void);
>   bool migration_is_running(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index 8c5bd0a75c85..49d11e1adf04 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1637,6 +1637,13 @@ bool migration_in_bg_snapshot(void)
>       return migrate_background_snapshot() && migration_is_running();
>   }
>
> +bool migration_is_none(void)
> +{
> +    MigrationState *s = current_migration;
> +
> +    return s->state == MIGRATION_STATUS_NONE;
> +}
> +
>   bool migration_is_active(void)
>   {
>       MigrationState *s = current_migration;
> --
> 2.39.3