1. Patchew
  2. QEMU
  3. [PATCH 0/6] vfio/migration: Block VFIO migration with postcopy and background snapshot
  4. View patch

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

Avihai Horon posted 6 patches 1 year, 3 months ago
There is a newer version of this series
[PATCH 1/6] migration: Add migration prefix to functions in target.c
Posted by Avihai Horon 1 year, 3 months ago 
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(-)

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
-- 
2.26.3
Re: [PATCH 1/6] migration: Add migration prefix to functions in target.c
Posted by Peter Xu 1 year, 3 months ago 
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;
>  }

-- 
Peter Xu
Re: [PATCH 1/6] migration: Add migration prefix to functions in target.c
Posted by Avihai Horon 1 year, 3 months ago 
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.
Re: [PATCH 1/6] migration: Add migration prefix to functions in target.c
Posted by Cédric Le Goater 1 year, 3 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.