Re: [PATCH 2/3] vfio/migration: Reset bytes_transferred properly

[Avihai Horon <avihaih@nvidia.com>]

On 26/06/2023 12:52, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> Hello Avihai,
>
> On 6/26/23 10:23, Avihai Horon wrote:
>> Currently, VFIO bytes_transferred is not reset properly:
>> 1. bytes_transferred is not reset after a VM snapshot (so a migration
>>     following a snapshot will report incorrect value).
>> 2. bytes_transferred is a single counter for all VFIO devices, however
>>     upon migration failure it is reset multiple times, by each VFIO
>>     device.
>>
>> Fix it by introducing a new function vfio_reset_bytes_transferred() and
>> calling it during migration and snapshot start.
>>
>> Remove existing bytes_transferred reset in VFIO migration state
>> notifier, which is not needed anymore.
>
> a Fixes: tag would be useful.

Sure, I will add.

>
>
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>> ---
>>   include/hw/vfio/vfio-common.h |  1 +
>>   migration/migration.h         |  1 +
>>   hw/vfio/migration.c           |  6 +++++-
>>   migration/migration.c         |  1 +
>>   migration/savevm.c            |  1 +
>>   migration/target.c            | 17 +++++++++++++++--
>>   6 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h 
>> b/include/hw/vfio/vfio-common.h
>> index 3dc5f2104c..b4c28f318f 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -228,6 +228,7 @@ int vfio_block_multiple_devices_migration(Error 
>> **errp);
>>   void vfio_unblock_multiple_devices_migration(void);
>>   int vfio_block_giommu_migration(Error **errp);
>>   int64_t vfio_mig_bytes_transferred(void);
>> +void vfio_reset_bytes_transferred(void);
>>
>>   #ifdef CONFIG_LINUX
>>   int vfio_get_region_info(VFIODevice *vbasedev, int index,
>> diff --git a/migration/migration.h b/migration/migration.h
>> index c859a0d35e..a80b22b703 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -514,6 +514,7 @@ bool migration_rate_limit(void);
>>   void migration_cancel(const Error *error);
>>
>>   void populate_vfio_info(MigrationInfo *info);
>> +void reset_vfio_bytes_transferred(void);
>>   void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
>>
>>   #endif
>> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index a8bfbe4b89..79eb81dfd7 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -704,7 +704,6 @@ static void 
>> vfio_migration_state_notifier(Notifier *notifier, void *data)
>>       case MIGRATION_STATUS_CANCELLING:
>>       case MIGRATION_STATUS_CANCELLED:
>>       case MIGRATION_STATUS_FAILED:
>> -        bytes_transferred = 0;
>>           /*
>>            * If setting the device in RUNNING state fails, the device 
>> should
>>            * be reset. To do so, use ERROR state as a recover state.
>> @@ -825,6 +824,11 @@ int64_t vfio_mig_bytes_transferred(void)
>>       return bytes_transferred;
>>   }
>>
>> +void vfio_reset_bytes_transferred(void)
>> +{
>> +    bytes_transferred = 0;
>> +}
>> +
>>   int vfio_migration_realize(VFIODevice *vbasedev, Error **errp)
>>   {
>>       int ret = -ENOTSUP;
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 7653787f74..096e8191d1 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1628,6 +1628,7 @@ static bool migrate_prepare(MigrationState *s, 
>> bool blk, bool blk_inc,
>>        */
>>       memset(&mig_stats, 0, sizeof(mig_stats));
>>       memset(&compression_counters, 0, sizeof(compression_counters));
>> +    reset_vfio_bytes_transferred();
>>
>>       return true;
>>   }
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index cdf4793924..95c2abf47c 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1622,6 +1622,7 @@ static int qemu_savevm_state(QEMUFile *f, Error 
>> **errp)
>>       migrate_init(ms);
>>       memset(&mig_stats, 0, sizeof(mig_stats));
>>       memset(&compression_counters, 0, sizeof(compression_counters));
>> +    reset_vfio_bytes_transferred();
>>       ms->to_dst_file = f;
>>
>>       qemu_mutex_unlock_iothread();
>> diff --git a/migration/target.c b/migration/target.c
>> index 00ca007f97..f39c9a8d88 100644
>> --- a/migration/target.c
>> +++ b/migration/target.c
>> @@ -14,12 +14,25 @@
>>   #include "hw/vfio/vfio-common.h"
>>   #endif
>>
>> +#ifdef CONFIG_VFIO
>>   void populate_vfio_info(MigrationInfo *info)
>>   {
>> -#ifdef CONFIG_VFIO
>>       if (vfio_mig_active()) {
>>           info->vfio = g_malloc0(sizeof(*info->vfio));
>>           info->vfio->transferred = vfio_mig_bytes_transferred();
>>       }
>> -#endif
>>   }
>> +
>> +void reset_vfio_bytes_transferred(void)
>> +{
>> +    vfio_reset_bytes_transferred();
>> +}
>> +#else
>> +void populate_vfio_info(MigrationInfo *info)
>> +{
>> +}
>> +
>> +void reset_vfio_bytes_transferred(void)
>> +{
>> +}
>> +#endif
>
> I would simply use static inline in migration.h if !CONFIG_VFIO.

I'm not sure it's possible to use CONFIG_VFIO in migration.h. Got this 
during compilation:
   migration/migration.h:517:8: error: attempt to use poisoned "CONFIG_VFIO"

Plus, see 43bd0bf30fce ("migration: Move populate_vfio_info() into a 
separate file") commit message:

     The CONFIG_VFIO switch only works in target specific code. Since
     migration/migration.c is common code, the #ifdef does not have
     the intended behavior here. Move the related code to a separate
     file now which gets compiled via specific_ss instead.

Or did I misunderstand you?

Thanks.