[v2] migration: Unify capabilities and parameters

[PATCH v2 06/24] migration: Run a post update routine after setting parameters

Posted by Fabiano Rosas 4 months, 2 weeks ago

Some migration parameters are updated immediately once they are set
via migrate-set-parameters. Move that work outside of
migrate_params_apply() and leave that function with the single
responsibility of setting s->parameters and not doing any
side-effects.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/options.c | 38 ++++++++++++++++++++++++++++----------
 migration/ram.c     |  2 +-
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 295367ce92..1f8a977865 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1083,6 +1083,31 @@ void migrate_params_init(MigrationParameters *params)
     params->has_direct_io = true;
 }
 
+static void migrate_post_update_params(MigrationParameters *new, Error **errp)
+{
+    MigrationState *s = migrate_get_current();
+
+    if (new->has_max_bandwidth) {
+        if (s->to_dst_file && !migration_in_postcopy()) {
+            migration_rate_set(new->max_bandwidth);
+        }
+    }
+
+    if (new->has_x_checkpoint_delay) {
+        colo_checkpoint_delay_set();
+    }
+
+    if (new->has_xbzrle_cache_size) {
+        xbzrle_cache_resize(new->xbzrle_cache_size, errp);
+    }
+
+    if (new->has_max_postcopy_bandwidth) {
+        if (s->to_dst_file && migration_in_postcopy()) {
+            migration_rate_set(new->max_postcopy_bandwidth);
+        }
+    }
+}
+
 /*
  * Check whether the parameters are valid. Error will be put into errp
  * (if provided). Return true if valid, otherwise false.
@@ -1393,7 +1418,7 @@ static void migrate_params_test_apply(MigrationParameters *params,
     }
 }
 
-static void migrate_params_apply(MigrationParameters *params, Error **errp)
+static void migrate_params_apply(MigrationParameters *params)
 {
     MigrationState *s = migrate_get_current();
 
@@ -1433,9 +1458,6 @@ static void migrate_params_apply(MigrationParameters *params, Error **errp)
 
     if (params->has_max_bandwidth) {
         s->parameters.max_bandwidth = params->max_bandwidth;
-        if (s->to_dst_file && !migration_in_postcopy()) {
-            migration_rate_set(s->parameters.max_bandwidth);
-        }
     }
 
     if (params->has_avail_switchover_bandwidth) {
@@ -1448,7 +1470,6 @@ static void migrate_params_apply(MigrationParameters *params, Error **errp)
 
     if (params->has_x_checkpoint_delay) {
         s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
-        colo_checkpoint_delay_set();
     }
 
     if (params->has_multifd_channels) {
@@ -1468,13 +1489,9 @@ static void migrate_params_apply(MigrationParameters *params, Error **errp)
     }
     if (params->has_xbzrle_cache_size) {
         s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
-        xbzrle_cache_resize(params->xbzrle_cache_size, errp);
     }
     if (params->has_max_postcopy_bandwidth) {
         s->parameters.max_postcopy_bandwidth = params->max_postcopy_bandwidth;
-        if (s->to_dst_file && migration_in_postcopy()) {
-            migration_rate_set(s->parameters.max_postcopy_bandwidth);
-        }
     }
     if (params->has_max_cpu_throttle) {
         s->parameters.max_cpu_throttle = params->max_cpu_throttle;
@@ -1542,7 +1559,8 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
     migrate_params_test_apply(params, &tmp);
 
     if (migrate_params_check(&tmp, errp)) {
-        migrate_params_apply(params, errp);
+        migrate_params_apply(params);
+        migrate_post_update_params(params, errp);
     }
 
     migrate_tls_opts_free(&tmp);
diff --git a/migration/ram.c b/migration/ram.c
index 2140785a05..7432f82bdd 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -174,7 +174,7 @@ static void XBZRLE_cache_unlock(void)
 /**
  * xbzrle_cache_resize: resize the xbzrle cache
  *
- * This function is called from migrate_params_apply in main
+ * This function is called from migrate_post_update_config in main
  * thread, possibly while a migration is in progress.  A running
  * migration may be using the cache and might finish during this call,
  * hence changes to the cache are protected by XBZRLE.lock().
-- 
2.35.3

re: [PATCH v2 06/24] migration: Run a post update routine after setting parameters

Posted by Bin Guo 1 month ago

Fabiano Rosas wrote on Mon, 30 Jun 2025 16:58:55 -0300:

> Some migration parameters are updated immediately once they are set
> via migrate-set-parameters. Move that work outside of
> migrate_params_apply() and leave that function with the single
> responsibility of setting s->parameters and not doing any
> side-effects.
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> 
> diff --git a/migration/options.c b/migration/options.c
> index 295367ce92..1f8a977865 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1083,6 +1083,31 @@ void migrate_params_init(MigrationParameters *params)
>      params->has_direct_io = true;
>  }
>  
> +static void migrate_post_update_params(MigrationParameters *new, Error **errp)
> +{
> +    MigrationState *s = migrate_get_current();
> +
> +    if (new->has_max_bandwidth) {
> +        if (s->to_dst_file && !migration_in_postcopy()) {
> +            migration_rate_set(new->max_bandwidth);
> +        }
> +    }
> +
> +    if (new->has_x_checkpoint_delay) {
> +        colo_checkpoint_delay_set();
> +    }
> +
> +    if (new->has_xbzrle_cache_size) {
> +        xbzrle_cache_resize(new->xbzrle_cache_size, errp);
> +    }
> +
> +    if (new->has_max_postcopy_bandwidth) {
> +        if (s->to_dst_file && migration_in_postcopy()) {
> +            migration_rate_set(new->max_postcopy_bandwidth);
> +        }
> +    }
> +}
> +
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 2140785a05..7432f82bdd 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -174,7 +174,7 @@ static void XBZRLE_cache_unlock(void)
>  /**
>   * xbzrle_cache_resize: resize the xbzrle cache
>   *
> - * This function is called from migrate_params_apply in main
> + * This function is called from migrate_post_update_config in main
>   * thread, possibly while a migration is in progress.  A running
>   * migration may be using the cache and might finish during this call,
>   * hence changes to the cache are protected by XBZRLE.lock().
> -- 

The function is migrate_post_update_params, 
but the comment says migrate_post_update_config.

--
Regards,
Bin Guo

re: [PATCH v2 06/24] migration: Run a post update routine after setting parameters

Posted by Fabiano Rosas 3 weeks, 1 day ago

Bin Guo <guobin@linux.alibaba.com> writes:

> Fabiano Rosas wrote on Mon, 30 Jun 2025 16:58:55 -0300:
>
>> Some migration parameters are updated immediately once they are set
>> via migrate-set-parameters. Move that work outside of
>> migrate_params_apply() and leave that function with the single
>> responsibility of setting s->parameters and not doing any
>> side-effects.
>> 
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> 
>> diff --git a/migration/options.c b/migration/options.c
>> index 295367ce92..1f8a977865 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -1083,6 +1083,31 @@ void migrate_params_init(MigrationParameters *params)
>>      params->has_direct_io = true;
>>  }
>>  
>> +static void migrate_post_update_params(MigrationParameters *new, Error **errp)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +
>> +    if (new->has_max_bandwidth) {
>> +        if (s->to_dst_file && !migration_in_postcopy()) {
>> +            migration_rate_set(new->max_bandwidth);
>> +        }
>> +    }
>> +
>> +    if (new->has_x_checkpoint_delay) {
>> +        colo_checkpoint_delay_set();
>> +    }
>> +
>> +    if (new->has_xbzrle_cache_size) {
>> +        xbzrle_cache_resize(new->xbzrle_cache_size, errp);
>> +    }
>> +
>> +    if (new->has_max_postcopy_bandwidth) {
>> +        if (s->to_dst_file && migration_in_postcopy()) {
>> +            migration_rate_set(new->max_postcopy_bandwidth);
>> +        }
>> +    }
>> +}
>> +
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 2140785a05..7432f82bdd 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -174,7 +174,7 @@ static void XBZRLE_cache_unlock(void)
>>  /**
>>   * xbzrle_cache_resize: resize the xbzrle cache
>>   *
>> - * This function is called from migrate_params_apply in main
>> + * This function is called from migrate_post_update_config in main
>>   * thread, possibly while a migration is in progress.  A running
>>   * migration may be using the cache and might finish during this call,
>>   * hence changes to the cache are protected by XBZRLE.lock().
>> -- 
>
> The function is migrate_post_update_params, 
> but the comment says migrate_post_update_config.
>

Nice catch, this series is a pain to rebase, a lot of cross-references
between patches. I'll fix.

> --
> Regards,
> Bin Guo