Re: [PATCH v4 5/6] migration: Support periodic RAMBlock dirty bitmap sync

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Oct 17, 2024 at 02:42:54PM +0800, yong.huang@smartx.com wrote:
>> +void cpu_throttle_dirty_sync_timer_tick(void *opaque)
>> +{
>> +    static uint64_t prev_sync_cnt;
>
> We may need to reset this in case migration got cancelled and invoked
> again, to make sure it keeps working in the 2nd run.
>
>> +    uint64_t sync_cnt = stat64_get(&mig_stats.dirty_sync_count);
>> +
>> +    /*
>> +     * 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);
>> +}
>
> Please both of you have a look on whether you agree I squash below into
> this patch when merge:
>
> ===8<===
> From 84a2544eab73e35dbd35fed3b1440169915f9aa4 Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Thu, 17 Oct 2024 15:27:19 -0400
> Subject: [PATCH] fixup! migration: Support periodic RAMBlock dirty bitmap sync
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/cpu-throttle.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/migration/cpu-throttle.c b/migration/cpu-throttle.c
> index 342681cdd4..3df287d8d3 100644
> --- a/migration/cpu-throttle.c
> +++ b/migration/cpu-throttle.c
> @@ -36,6 +36,7 @@
>  static QEMUTimer *throttle_timer, *throttle_dirty_sync_timer;
>  static unsigned int throttle_percentage;
>  static bool throttle_dirty_sync_timer_active;
> +static uint64_t throttle_dirty_sync_count_prev;
>  
>  #define CPU_THROTTLE_PCT_MIN 1
>  #define CPU_THROTTLE_PCT_MAX 99
> @@ -133,7 +134,6 @@ int cpu_throttle_get_percentage(void)
>  
>  void cpu_throttle_dirty_sync_timer_tick(void *opaque)
>  {
> -    static uint64_t prev_sync_cnt;
>      uint64_t sync_cnt = stat64_get(&mig_stats.dirty_sync_count);
>  
>      /*
> @@ -145,7 +145,7 @@ void cpu_throttle_dirty_sync_timer_tick(void *opaque)
>          goto end;
>      }
>  
> -    if (sync_cnt == prev_sync_cnt) {
> +    if (sync_cnt == throttle_dirty_sync_count_prev) {
>          trace_cpu_throttle_dirty_sync();
>          WITH_RCU_READ_LOCK_GUARD() {
>              migration_bitmap_sync_precopy(false);
> @@ -153,7 +153,7 @@ void cpu_throttle_dirty_sync_timer_tick(void *opaque)
>      }
>  
>  end:
> -    prev_sync_cnt = stat64_get(&mig_stats.dirty_sync_count);
> +    throttle_dirty_sync_count_prev = stat64_get(&mig_stats.dirty_sync_count);
>  
>      timer_mod(throttle_dirty_sync_timer,
>          qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) +
> @@ -171,6 +171,11 @@ void cpu_throttle_dirty_sync_timer(bool enable)
>  
>      if (enable) {
>          if (!cpu_throttle_dirty_sync_active()) {
> +            /*
> +             * Always reset the dirty sync count cache, in case migration
> +             * was cancelled once.
> +             */
> +            throttle_dirty_sync_count_prev = 0;
>              timer_mod(throttle_dirty_sync_timer,
>                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL_RT) +
>                      CPU_THROTTLE_DIRTY_SYNC_TIMESLICE_MS);
> -- 
> 2.45.0

LGTM