On Thu, Jul 15, 2021 at 11:51:33PM +0800, huang...@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huang...@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(黄勇) <huang...@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
