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

[Joao Martins]

On 16/12/2024 12:32, Joao Martins wrote:
> 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.
> 
> 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. What would be the alternative 
> idea
> forward?
> 
> -------------------->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;
>      }
> 

NB: The patch is obviously incomplete given that I missed the check in
vfio_devices_all_running_and_mig_active()

> 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