Re: [PATCH v3 1/4] migration: Move cpu-throttole.c from system to migration

[Peter Xu <peterx@redhat.com>]

On Wed, Oct 16, 2024 at 03:56:42PM +0800, yong.huang@smartx.com wrote:
> From: Hyman Huang <yong.huang@smartx.com>
> 
> Move cpu-throttle.c from system to migration since it's
> only used for migration; this makes us avoid exporting the
> util functions and variables in misc.h but export them in
> migration.h when implementing the background ramblock dirty
> sync feature in the upcoming commits.
> 
> Additionally, make the two modifications below:
> 
> 1. Delay the timer registering of CPU throttle until
>    migration starts since it is only used in migration.
> 
> 2. Stop CPU throttle if auto converge capability is
>    enabled since it only happens with auto converge.
> 
> 3. Remove the unused header file reference in
>    accel/tcg/icount-common.c.

Please consider split the things into smaller patches, especially when it
involves file movements.

> 
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> ---
>  accel/tcg/icount-common.c                    |  1 -
>  {system => migration}/cpu-throttle.c         |  2 +-
>  {include/sysemu => migration}/cpu-throttle.h |  0
>  migration/meson.build                        |  1 +
>  migration/migration.c                        | 11 +++++++++--
>  migration/ram.c                              |  2 +-
>  migration/trace-events                       |  3 +++
>  system/cpu-timers.c                          |  3 ---
>  system/meson.build                           |  1 -
>  system/trace-events                          |  3 ---
>  10 files changed, 15 insertions(+), 12 deletions(-)
>  rename {system => migration}/cpu-throttle.c (99%)
>  rename {include/sysemu => migration}/cpu-throttle.h (100%)
> 
> diff --git a/accel/tcg/icount-common.c b/accel/tcg/icount-common.c
> index 8d3d3a7e9d..30bf8500dc 100644
> --- a/accel/tcg/icount-common.c
> +++ b/accel/tcg/icount-common.c
> @@ -36,7 +36,6 @@
>  #include "sysemu/runstate.h"
>  #include "hw/core/cpu.h"
>  #include "sysemu/cpu-timers.h"
> -#include "sysemu/cpu-throttle.h"
>  #include "sysemu/cpu-timers-internal.h"
>  
>  /*
> diff --git a/system/cpu-throttle.c b/migration/cpu-throttle.c
> similarity index 99%
> rename from system/cpu-throttle.c
> rename to migration/cpu-throttle.c
> index 7632dc6143..fa47ee2e21 100644
> --- a/system/cpu-throttle.c
> +++ b/migration/cpu-throttle.c
> @@ -27,7 +27,7 @@
>  #include "hw/core/cpu.h"
>  #include "qemu/main-loop.h"
>  #include "sysemu/cpus.h"
> -#include "sysemu/cpu-throttle.h"
> +#include "cpu-throttle.h"
>  #include "trace.h"
>  
>  /* vcpu throttling controls */
> diff --git a/include/sysemu/cpu-throttle.h b/migration/cpu-throttle.h
> similarity index 100%
> rename from include/sysemu/cpu-throttle.h
> rename to migration/cpu-throttle.h
> diff --git a/migration/meson.build b/migration/meson.build
> index 66d3de86f0..d53cf3417a 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -13,6 +13,7 @@ system_ss.add(files(
>    'block-dirty-bitmap.c',
>    'channel.c',
>    'channel-block.c',
> +  'cpu-throttle.c',
>    'dirtyrate.c',
>    'exec.c',
>    'fd.c',
> diff --git a/migration/migration.c b/migration/migration.c
> index 021faee2f3..7e71184257 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -24,7 +24,7 @@
>  #include "socket.h"
>  #include "sysemu/runstate.h"
>  #include "sysemu/sysemu.h"
> -#include "sysemu/cpu-throttle.h"
> +#include "cpu-throttle.h"
>  #include "rdma.h"
>  #include "ram.h"
>  #include "migration/global_state.h"
> @@ -3289,7 +3289,9 @@ static MigIterateState migration_iteration_run(MigrationState *s)
>  static void migration_iteration_finish(MigrationState *s)
>  {
>      /* If we enabled cpu throttling for auto-converge, turn it off. */
> -    cpu_throttle_stop();
> +    if (migrate_auto_converge()) {
> +        cpu_throttle_stop();
> +    }
>  
>      bql_lock();
>      switch (s->state) {
> @@ -3508,6 +3510,11 @@ static void *migration_thread(void *opaque)
>          qemu_savevm_send_colo_enable(s->to_dst_file);
>      }
>  
> +    if (migrate_auto_converge()) {
> +        /* Start cpu throttle timers */
> +        cpu_throttle_init();
> +    }

Might this leak the timer object? 

I think it perhaps needs to be moved to migration_object_init().

> +
>      bql_lock();
>      ret = qemu_savevm_state_setup(s->to_dst_file, &local_err);
>      bql_unlock();
> diff --git a/migration/ram.c b/migration/ram.c
> index 326ce7eb79..54d352b152 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -52,7 +52,7 @@
>  #include "exec/target_page.h"
>  #include "qemu/rcu_queue.h"
>  #include "migration/colo.h"
> -#include "sysemu/cpu-throttle.h"
> +#include "cpu-throttle.h"
>  #include "savevm.h"
>  #include "qemu/iov.h"
>  #include "multifd.h"
> diff --git a/migration/trace-events b/migration/trace-events
> index c65902f042..9a19599804 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -378,3 +378,6 @@ migration_block_progression(unsigned percent) "Completed %u%%"
>  # page_cache.c
>  migration_pagecache_init(int64_t max_num_items) "Setting cache buckets to %" PRId64
>  migration_pagecache_insert(void) "Error allocating page"
> +
> +# cpu-throttle.c
> +cpu_throttle_set(int new_throttle_pct)  "set guest CPU throttled by %d%%"
> diff --git a/system/cpu-timers.c b/system/cpu-timers.c
> index 0b31c9a1b6..856e502e34 100644
> --- a/system/cpu-timers.c
> +++ b/system/cpu-timers.c
> @@ -35,7 +35,6 @@
>  #include "sysemu/runstate.h"
>  #include "hw/core/cpu.h"
>  #include "sysemu/cpu-timers.h"
> -#include "sysemu/cpu-throttle.h"
>  #include "sysemu/cpu-timers-internal.h"
>  
>  /* clock and ticks */
> @@ -272,6 +271,4 @@ void cpu_timers_init(void)
>      seqlock_init(&timers_state.vm_clock_seqlock);
>      qemu_spin_init(&timers_state.vm_clock_lock);
>      vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
> -
> -    cpu_throttle_init();
>  }
> diff --git a/system/meson.build b/system/meson.build
> index a296270cb0..4952f4b2c7 100644
> --- a/system/meson.build
> +++ b/system/meson.build
> @@ -10,7 +10,6 @@ system_ss.add(files(
>    'balloon.c',
>    'bootdevice.c',
>    'cpus.c',
> -  'cpu-throttle.c',
>    'cpu-timers.c',
>    'datadir.c',
>    'dirtylimit.c',
> diff --git a/system/trace-events b/system/trace-events
> index 074d001e90..2ed1d59b1f 100644
> --- a/system/trace-events
> +++ b/system/trace-events
> @@ -44,6 +44,3 @@ dirtylimit_state_finalize(void)
>  dirtylimit_throttle_pct(int cpu_index, uint64_t pct, int64_t time_us) "CPU[%d] throttle percent: %" PRIu64 ", throttle adjust time %"PRIi64 " us"
>  dirtylimit_set_vcpu(int cpu_index, uint64_t quota) "CPU[%d] set dirty page rate limit %"PRIu64
>  dirtylimit_vcpu_execute(int cpu_index, int64_t sleep_time_us) "CPU[%d] sleep %"PRIi64 " us"
> -
> -# cpu-throttle.c
> -cpu_throttle_set(int new_throttle_pct)  "set guest CPU throttled by %d%%"
> -- 
> 2.27.0
> 

-- 
Peter Xu