Re: [PATCH v3 3/3] migration/dirtyrate: implement dirty-bitmap dirtyrate calculation

[Peter Xu <peterx@redhat.com>]

On Thu, Jul 15, 2021 at 11:51:33PM +0800, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> introduce dirty-bitmap mode as the third method of calc-dirty-rate.
> implement dirty-bitmap dirtyrate calculation, which can be used
> to measuring dirtyrate in the absence of dirty-ring.
> 
> introduce "dirty_bitmap:-b" option in hmp calc_dirty_rate to
> indicate dirty bitmap method should be used for calculation.
> 
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> ---
>  hmp-commands.hx        |   9 ++--
>  migration/dirtyrate.c  | 116 +++++++++++++++++++++++++++++++++++++++++++++----
>  migration/trace-events |   1 +
>  qapi/migration.json    |   6 ++-
>  4 files changed, 117 insertions(+), 15 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f7fc9d7..605973c 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1738,9 +1738,10 @@ ERST
>  
>      {
>          .name       = "calc_dirty_rate",
> -        .args_type  = "dirty_ring:-r,second:l,sample_pages_per_GB:l?",
> -        .params     = "[-r] second [sample_pages_per_GB]",
> -        .help       = "start a round of guest dirty rate measurement (using -d to"
> -                      "\n\t\t\t specify dirty ring as the method of calculation)",
> +        .args_type  = "dirty_ring:-r,dirty_bitmap:-b,second:l,sample_pages_per_GB:l?",
> +        .params     = "[-r] [-b] second [sample_pages_per_GB]",
> +        .help       = "start a round of guest dirty rate measurement (using -r to"
> +                      "\n\t\t\t specify dirty ring as the method of calculation and"
> +                      "\n\t\t\t -b to specify dirty bitmap as method of calculation)",
>          .cmd        = hmp_calc_dirty_rate,
>      },
> diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
> index c465e62..8006a0d 100644
> --- a/migration/dirtyrate.c
> +++ b/migration/dirtyrate.c
> @@ -15,6 +15,7 @@
>  #include "qapi/error.h"
>  #include "cpu.h"
>  #include "exec/ramblock.h"
> +#include "exec/ram_addr.h"
>  #include "qemu/rcu_queue.h"
>  #include "qemu/main-loop.h"
>  #include "qapi/qapi-commands-migration.h"
> @@ -113,6 +114,10 @@ static struct DirtyRateInfo *query_dirty_rate_info(void)
>              }
>              info->vcpu_dirty_rate = head;
>          }
> +
> +        if (dirtyrate_mode == DIRTY_RATE_MEASURE_MODE_DIRTY_BITMAP) {
> +            info->sample_pages = 0;
> +        }
>      }
>  
>      trace_query_dirty_rate_info(DirtyRateStatus_str(CalculatingState));
> @@ -410,6 +415,83 @@ static void dirtyrate_global_dirty_log_stop(void)
>      qemu_mutex_unlock_iothread();
>  }
>  
> +static inline void dirtyrate_dirtypages_clear(void)
> +{
> +    DirtyRateDirtyPages = 0;

I think this is okay; but I think it's easier we keep it increase-only, so we
do things similar to dirty ring - we record the diff before/after.

> +}
> +
> +static inline void dirtyrate_manual_reset_protect(void)
> +{
> +    RAMBlock *block = NULL;
> +
> +    WITH_RCU_READ_LOCK_GUARD() {
> +        RAMBLOCK_FOREACH_MIGRATABLE(block) {
> +            cpu_physical_memory_dirtyrate_reset_protect(block);
> +        }
> +    }
> +}
> +
> +static void do_calculate_dirtyrate_bitmap(void)
> +{
> +    uint64_t memory_size_MB;
> +    int64_t time_s;
> +    uint64_t dirtyrate = 0;
> +
> +    memory_size_MB = (DirtyRateDirtyPages *TARGET_PAGE_SIZE) >> 20;
> +    time_s = DirtyStat.calc_time;
> +
> +    dirtyrate = memory_size_MB / time_s;
> +    DirtyStat.dirty_rate = dirtyrate;
> +
> +    trace_dirtyrate_do_calculate_bitmap(DirtyRateDirtyPages,
> +                                        time_s, dirtyrate);
> +}
> +
> +static void calculate_dirtyrate_dirty_bitmap(struct DirtyRateConfig config)
> +{
> +    int64_t msec = 0;
> +    int64_t start_time;
> +    uint64_t protect_flags = kvm_get_manual_dirty_log_protect();
> +
> +    dirtyrate_global_dirty_log_start();
> +
> +    /*
> +     * 1'round of log sync may return all 1 bits with
> +     * KVM_DIRTY_LOG_INITIALLY_SET enable
> +     * skip it unconditionally and start dirty tracking
> +     * from 2'round of log sync
> +     */
> +    memory_global_dirty_log_sync();

I think we need BQL for calling this.  E.g., memory_listeners can grow as we
call, also I'm not sure all ->log_sync() hooks are thread-safe.

> +
> +    /*
> +     * reset page protect manually and
> +     * start dirty tracking from now on
> +     */
> +    if (protect_flags & KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE) {
> +        dirtyrate_manual_reset_protect();

I think this needs BQL too.

Meanwhile after a second thought I think it's easier to drop patch 1 and call
dirtyrate_manual_reset_protect() unconditionally.  Say, kvm is not the only one
that may need a log clear (or say, re-protect), so checking kvm-only is not
actually a "complete" solution, because when there're more log_clear() hooks we
may want to call them too.

All modules providing log_clear() hook should be able to handle this correctly,
e.g., kvm_log_clear() can be called even without manual protect enabled, see
entry of kvm_physical_log_clear() which checks manual_dirty_log_protect.

> +    }
> +
> +    dirtyrate_dirtypages_clear();
> +
> +    start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    DirtyStat.start_time = start_time / 1000;
> +
> +    msec = config.sample_period_seconds * 1000;
> +    msec = set_sample_page_period(msec, start_time);
> +    DirtyStat.calc_time = msec / 1000;
> +
> +    /* fetch dirty bitmap from kvm */
> +    memory_global_dirty_log_sync();
> +
> +    do_calculate_dirtyrate_bitmap();
> +
> +    if (protect_flags & KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE) {
> +        dirtyrate_manual_reset_protect();
> +    }

I feel like this chunk can be dropped.

> +
> +    dirtyrate_global_dirty_log_stop();
> +}

Thanks,

-- 
Peter Xu