On Wed, Oct 16, 2024 at 03:56:44PM +0800, yong.hu...@smartx.com wrote:
> From: Hyman Huang <yong.hu...@smartx.com>
>
> When VM is configured with huge memory, the current throttle logic
> doesn't look like to scale, because migration_trigger_throttle()
> is only called for each iteration, so it won't be invoked for a long
> time if one iteration can take a long time.
>
> The periodic dirty sync aims to fix the above issue by synchronizing
> the ramblock from remote dirty bitmap and, when necessary, triggering
> the CPU throttle multiple times during a long iteration.
>
> This is a trade-off between synchronization overhead and CPU throttle
> impact.
>
> Signed-off-by: Hyman Huang <yong.hu...@smartx.com>
> ---
> migration/cpu-throttle.c | 70 +++++++++++++++++++++++++++++++++++++++-
> migration/cpu-throttle.h | 14 ++++++++
> migration/migration.h | 1 +
> migration/ram.c | 9 ++++--
> migration/trace-events | 1 +
> 5 files changed, 92 insertions(+), 3 deletions(-)
>
> diff --git a/migration/cpu-throttle.c b/migration/cpu-throttle.c
> index fa47ee2e21..784b51ab35 100644
> --- a/migration/cpu-throttle.c
> +++ b/migration/cpu-throttle.c
> @@ -28,16 +28,23 @@
> #include "qemu/main-loop.h"
> #include "sysemu/cpus.h"
> #include "cpu-throttle.h"
> +#include "migration.h"
> +#include "migration-stats.h"
> +#include "options.h"
> #include "trace.h"
>
> /* vcpu throttling controls */
> -static QEMUTimer *throttle_timer;
> +static QEMUTimer *throttle_timer, *throttle_dirty_sync_timer;
> static unsigned int throttle_percentage;
> +static bool throttle_dirty_sync_timer_active;
>
> #define CPU_THROTTLE_PCT_MIN 1
> #define CPU_THROTTLE_PCT_MAX 99
> #define CPU_THROTTLE_TIMESLICE_NS 10000000
>
> +/* RAMBlock dirty sync trigger every five seconds */
Maybe enrich it to say "making sure it is synchronized every five seconds"?
Because it can synchronize faster if each iteration runs faster than 5sec,
so just to emphasize it's a fallback sync, and only with auto converge.
> +#define CPU_THROTTLE_DIRTY_SYNC_TIMESLICE_MS 5000
> +
> static void cpu_throttle_thread(CPUState *cpu, run_on_cpu_data opaque)
> {
> double pct;
> @@ -112,6 +119,7 @@ void cpu_throttle_set(int new_throttle_pct)
> void cpu_throttle_stop(void)
> {
> qatomic_set(&throttle_percentage, 0);
> + cpu_throttle_dirty_sync_timer(false);
> }
>
> bool cpu_throttle_active(void)
> @@ -124,8 +132,68 @@ int cpu_throttle_get_percentage(void)
> return qatomic_read(&throttle_percentage);
> }
>
> +void cpu_throttle_dirty_sync_timer_tick(void *opaque)
> +{
> + static uint64_t prev_sync_cnt = 2;
IIUC the hard coded "2" isn't needed, as long as it's guaranteed to be
updated for each timer call, and you special cased "1" anyway below.
> + uint64_t sync_cnt = stat64_get(&mig_stats.dirty_sync_count);
> +
> + if (!migrate_auto_converge()) {
> + /* Stop the timer when auto converge is disabled */
> + return;
I think we can try to make sure this never starts if !auto-converge, so
assuming this path will never trigger in real life.
> + }
> +
> + /*
> + * The first iteration copies all memory anyhow and has no
> + * effect on guest performance, therefore omit it to avoid
> + * paying extra for the sync penalty.
> + */
> + if (sync_cnt <= 1) {
> + goto end;
> + }
> +
> + if (sync_cnt == prev_sync_cnt) {
> + trace_cpu_throttle_dirty_sync();
> + WITH_RCU_READ_LOCK_GUARD() {
> + migration_bitmap_sync_precopy(false);
> + }
> + }
> +
> +end:
> + prev_sync_cnt = stat64_get(&mig_stats.dirty_sync_count);
> +
> + timer_mod(throttle_dirty_sync_timer,
> + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) +
> + CPU_THROTTLE_DIRTY_SYNC_TIMESLICE_MS);
> +}
> +
> +static bool cpu_throttle_dirty_sync_active(void)
> +{
> + return qatomic_read(&throttle_dirty_sync_timer_active);
> +}
> +
> +void cpu_throttle_dirty_sync_timer(bool enable)
> +{
> + if (enable) {
> + assert(throttle_dirty_sync_timer);
> + if (!cpu_throttle_dirty_sync_active()) {
I suppose this can be logically racy? As I think after this patch this path
can be invoked both in main thread and migration thread.
The simplest way to do is to move cpu_throttle_stop() call to be under
bql_lock(), so that this will be serialized by BQL. Then we can add an
assertion at the entry of the function for bql_locked().
> + timer_mod(throttle_dirty_sync_timer,
> + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) +
> + CPU_THROTTLE_DIRTY_SYNC_TIMESLICE_MS);
> + qatomic_set(&throttle_dirty_sync_timer_active, 1);
> + }
> + } else {
> + if (throttle_dirty_sync_timer != NULL) {
IIUC throttle_dirty_sync_timer is never destroyed, aka, timer_del() only
disables it. So you should probably use throttle_dirty_sync_timer_active?
> + timer_del(throttle_dirty_sync_timer);
> + qatomic_set(&throttle_dirty_sync_timer_active, 0);
> + }
> + }
> +}
> +
> void cpu_throttle_init(void)
> {
> throttle_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL_RT,
> cpu_throttle_timer_tick, NULL);
> + throttle_dirty_sync_timer =
> + timer_new_ms(QEMU_CLOCK_VIRTUAL_RT,
> + cpu_throttle_dirty_sync_timer_tick, NULL);
> }
> diff --git a/migration/cpu-throttle.h b/migration/cpu-throttle.h
> index d65bdef6d0..420702b8d3 100644
> --- a/migration/cpu-throttle.h
> +++ b/migration/cpu-throttle.h
> @@ -65,4 +65,18 @@ bool cpu_throttle_active(void);
> */
> int cpu_throttle_get_percentage(void);
>
> +/**
> + * cpu_throttle_dirty_sync_timer_tick:
> + *
> + * Dirty sync timer hook.
> + */
> +void cpu_throttle_dirty_sync_timer_tick(void *opaque);
> +
> +/**
> + * cpu_throttle_dirty_sync_timer:
> + *
> + * Start or stop the dirty sync timer.
> + */
> +void cpu_throttle_dirty_sync_timer(bool enable);
> +
> #endif /* SYSEMU_CPU_THROTTLE_H */
> diff --git a/migration/migration.h b/migration/migration.h
> index 38aa1402d5..fbd0d19092 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -537,4 +537,5 @@ int migration_rp_wait(MigrationState *s);
> */
> void migration_rp_kick(MigrationState *s);
>
> +void migration_bitmap_sync_precopy(bool last_stage);
> #endif
> diff --git a/migration/ram.c b/migration/ram.c
> index 9b5b350405..ac34e731e2 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1020,6 +1020,11 @@ static void migration_trigger_throttle(RAMState *rs)
> migration_transferred_bytes() - rs->bytes_xfer_prev;
> uint64_t bytes_dirty_period = rs->num_dirty_pages_period *
> TARGET_PAGE_SIZE;
> uint64_t bytes_dirty_threshold = bytes_xfer_period * threshold / 100;
> + bool auto_converge = migrate_auto_converge();
> +
> + if (auto_converge) {
> + cpu_throttle_dirty_sync_timer(true);
> + }
If you have the guard to skip the 1st sync in the timer fn(), IIUC you can
move this earlier to e.g. migration_thread() before iteration starts.
Otherwise it'll be not as clear on when this timer will start if it hides
in the sync path itself.
>
> /*
> * The following detection logic can be refined later. For now:
> @@ -1031,7 +1036,7 @@ static void migration_trigger_throttle(RAMState *rs)
> if ((bytes_dirty_period > bytes_dirty_threshold) &&
> (++rs->dirty_rate_high_cnt >= 2)) {
> rs->dirty_rate_high_cnt = 0;
> - if (migrate_auto_converge()) {
> + if (auto_converge) {
> trace_migration_throttle();
> mig_throttle_guest_down(bytes_dirty_period,
> bytes_dirty_threshold);
> @@ -1088,7 +1093,7 @@ static void migration_bitmap_sync(RAMState *rs, bool
> last_stage)
> }
> }
>
> -static void migration_bitmap_sync_precopy(bool last_stage)
> +void migration_bitmap_sync_precopy(bool last_stage)
> {
> Error *local_err = NULL;
> assert(ram_state);
> diff --git a/migration/trace-events b/migration/trace-events
> index 9a19599804..0638183056 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -381,3 +381,4 @@ migration_pagecache_insert(void) "Error allocating page"
>
> # cpu-throttle.c
> cpu_throttle_set(int new_throttle_pct) "set guest CPU throttled by %d%%"
> +cpu_throttle_dirty_sync(void) ""
> --
> 2.27.0
>
--
Peter Xu
