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

["Cédric Le Goater" <clg@redhat.com>]

On 6/26/23 13:46, Avihai Horon wrote:
> 
> 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.

You could reply with the appropriate tag and the tooling will automatically
collect it. No need to resend for that.
  
> 
>>
>>
>>> 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?

I didn't realize that was not possible. Leave it that way then.

Thanks,

C.