| Message ID | 20230828151842.11303-2-avihaih@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio/migration: Block VFIO migration with postcopy and background snapshot | expand |
On 8/28/23 17:18, Avihai Horon wrote: > The functions in target.c are not static, yet they don't have a proper > migration prefix. Add such prefix. Reviewed-by: Cédric Le Goater <clg@redhat.com> Thanks, C. > > Signed-off-by: Avihai Horon <avihaih@nvidia.com> > --- > 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); > 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(); > > return true; > } > diff --git a/migration/savevm.c b/migration/savevm.c > index a2cb8855e2..5bf8b59a7d 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -1622,7 +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(); > + migration_reset_vfio_bytes_transferred(); > ms->to_dst_file = f; > > qemu_mutex_unlock_iothread(); > diff --git a/migration/target.c b/migration/target.c > index f39c9a8d88..a6ffa9a5ce 100644 > --- a/migration/target.c > +++ b/migration/target.c > @@ -15,7 +15,7 @@ > #endif > > #ifdef CONFIG_VFIO > -void populate_vfio_info(MigrationInfo *info) > +void migration_populate_vfio_info(MigrationInfo *info) > { > if (vfio_mig_active()) { > info->vfio = g_malloc0(sizeof(*info->vfio)); > @@ -23,16 +23,16 @@ void populate_vfio_info(MigrationInfo *info) > } > } > > -void reset_vfio_bytes_transferred(void) > +void migration_reset_vfio_bytes_transferred(void) > { > vfio_reset_bytes_transferred(); > } > #else > -void populate_vfio_info(MigrationInfo *info) > +void migration_populate_vfio_info(MigrationInfo *info) > { > } > > -void reset_vfio_bytes_transferred(void) > +void migration_reset_vfio_bytes_transferred(void) > { > } > #endif
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) > 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. Thanks, > > return true; > }
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.
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); 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(); return true; } diff --git a/migration/savevm.c b/migration/savevm.c index a2cb8855e2..5bf8b59a7d 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1622,7 +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(); + migration_reset_vfio_bytes_transferred(); ms->to_dst_file = f; qemu_mutex_unlock_iothread(); diff --git a/migration/target.c b/migration/target.c index f39c9a8d88..a6ffa9a5ce 100644 --- a/migration/target.c +++ b/migration/target.c @@ -15,7 +15,7 @@ #endif #ifdef CONFIG_VFIO -void populate_vfio_info(MigrationInfo *info) +void migration_populate_vfio_info(MigrationInfo *info) { if (vfio_mig_active()) { info->vfio = g_malloc0(sizeof(*info->vfio)); @@ -23,16 +23,16 @@ void populate_vfio_info(MigrationInfo *info) } } -void reset_vfio_bytes_transferred(void) +void migration_reset_vfio_bytes_transferred(void) { vfio_reset_bytes_transferred(); } #else -void populate_vfio_info(MigrationInfo *info) +void migration_populate_vfio_info(MigrationInfo *info) { } -void reset_vfio_bytes_transferred(void) +void migration_reset_vfio_bytes_transferred(void) { } #endif
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> --- migration/migration.h | 4 ++-- migration/migration.c | 6 +++--- migration/savevm.c | 2 +- migration/target.c | 8 ++++---- 4 files changed, 10 insertions(+), 10 deletions(-)