Re: [PATCH 1/6] migration: Add migration prefix to functions in target.c

[Avihai Horon]

On 29/08/2023 17:04, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Aug 28, 2023 at 06:18:37PM +0300, Avihai Horon wrote:
> > The functions in target.c are not static, yet they don't have a proper
> > migration prefix. Add such prefix.
> >
> > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> No issue on the patch itself, but just noticed that we have hard-coded vfio
> calls in migration paths.. that's slightly unfortunate. :(
>
> > ---
> >   migration/migration.h | 4 ++--
> >   migration/migration.c | 6 +++---
> >   migration/savevm.c    | 2 +-
> >   migration/target.c    | 8 ++++----
> >   4 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 6eea18db36..c5695de214 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -512,8 +512,8 @@ void migration_consume_urgent_request(void);
> >   bool migration_rate_limit(void);
> >   void migration_cancel(const Error *error);
> >
> > -void populate_vfio_info(MigrationInfo *info);
> > -void reset_vfio_bytes_transferred(void);
> > +void migration_populate_vfio_info(MigrationInfo *info);
> > +void migration_reset_vfio_bytes_transferred(void);
> >   void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
> >
> >   #endif
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 5528acb65e..92866a8f49 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1039,7 +1039,7 @@ static void fill_source_migration_info(MigrationInfo 
> > *info)
> >           populate_time_info(info, s);
> >           populate_ram_info(info, s);
> >           populate_disk_info(info);
> > -        populate_vfio_info(info);
> > +        migration_populate_vfio_info(info);
> (maybe in the future we should have SaveVMHandlers hooks for populating
>   data for ram/disk/vfio/..., or some other way to not hard-code these)

This sounds like a good idea.

> >           break;
> >       case MIGRATION_STATUS_COLO:
> >           info->has_status = true;
> > @@ -1048,7 +1048,7 @@ static void fill_source_migration_info(MigrationInfo 
> > *info)
> >       case MIGRATION_STATUS_COMPLETED:
> >           populate_time_info(info, s);
> >           populate_ram_info(info, s);
> > -        populate_vfio_info(info);
> > +        migration_populate_vfio_info(info);
> >           break;
> >       case MIGRATION_STATUS_FAILED:
> >           info->has_status = true;
> > @@ -1641,7 +1641,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();
> > +    migration_reset_vfio_bytes_transferred();
> Could this already be done during vfio_save_setup(), to avoid calling an
> vfio function directly in migration.c?
>
> Again, not a request for this patchset, but more to see whether it'll work
> to be moved there.

VFIO bytes_transferred is a global variable for all VFIO devices.
Resetting it in vfio_save_setup() means that if there are multiple VFIO 
devices, it will be reset multiple times and that doesn't seem clean IMHO.

But maybe it would be a good idea to extend the VFIO migration info: 
make it per device and maybe split it to pre-copy data, stop-copy data, etc.
Then we can reset it in vfio_save_setup().

Thanks.