Hyman Huang <[email protected]> writes:
> shadow_bmap, iter_bmap and iter_dirty_pages are introduced
> to satisfy the need for background sync.
>
> Meanwhile, introduce enumeration of sync method.
>
> Signed-off-by: Hyman Huang <[email protected]>
> ---
> include/exec/ramblock.h | 45 +++++++++++++++++++++++++++++++++++++++++
> migration/ram.c | 6 ++++++
> 2 files changed, 51 insertions(+)
>
> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
> index 0babd105c0..0e327bc0ae 100644
> --- a/include/exec/ramblock.h
> +++ b/include/exec/ramblock.h
> @@ -24,6 +24,30 @@
> #include "qemu/rcu.h"
> #include "exec/ramlist.h"
>
> +/* Possible bits for cpu_physical_memory_sync_dirty_bitmap */
> +
> +/*
> + * The old-fashioned sync, which is, in turn, used for CPU
> + * throttle and memory transfer.
I'm not sure I follow what "in turn" is supposed to mean in this
sentence. Could you clarify?
> + */
> +#define RAMBLOCK_SYN_LEGACY_ITER (1U << 0)
So ITER is as opposed to background? I'm a bit confused with the terms.
> +
> +/*
> + * The modern sync, which is, in turn, used for CPU throttle
> + * and memory transfer.
> + */
> +#define RAMBLOCK_SYN_MODERN_ITER (1U << 1)
> +
> +/* The modern sync, which is used for CPU throttle only */
> +#define RAMBLOCK_SYN_MODERN_BACKGROUND (1U << 2)
What's the plan for the "legacy" part? To be removed soon? Do we want to
remove it now? Maybe better to not use the modern/legacy terms unless we
want to give the impression that the legacy one is discontinued.
> +
> +#define RAMBLOCK_SYN_MASK (0x7)
> +
> +typedef enum RAMBlockSynMode {
> + RAMBLOCK_SYN_LEGACY, /* Old-fashined mode */
> + RAMBLOCK_SYN_MODERN, /* Background-sync-supported mode */
> +} RAMBlockSynMode;
I'm also wondering wheter we need this enum + the flags or one of them
would suffice. I'm looking at code like this in the following patches,
for instance:
+ if (sync_mode == RAMBLOCK_SYN_MODERN) {
+ if (background) {
+ flag = RAMBLOCK_SYN_MODERN_BACKGROUND;
+ } else {
+ flag = RAMBLOCK_SYN_MODERN_ITER;
+ }
+ }
Couldn't we use LEGACY/BG/ITER?
> +
> struct RAMBlock {
> struct rcu_head rcu;
> struct MemoryRegion *mr;
> @@ -89,6 +113,27 @@ struct RAMBlock {
> * could not have been valid on the source.
> */
> ram_addr_t postcopy_length;
> +
> + /*
> + * Used to backup the bmap during background sync to see whether any
> dirty
> + * pages were sent during that time.
> + */
> + unsigned long *shadow_bmap;
> +
> + /*
> + * The bitmap "bmap," which was initially used for both sync and memory
> + * transfer, will be replaced by two bitmaps: the previously used "bmap"
> + * and the recently added "iter_bmap." Only the memory transfer is
> + * conducted with the previously used "bmap"; the recently added
> + * "iter_bmap" is utilized for dirty bitmap sync.
> + */
> + unsigned long *iter_bmap;
> +
> + /* Number of new dirty pages during iteration */
> + uint64_t iter_dirty_pages;
> +
> + /* If background sync has shown up during iteration */
> + bool background_sync_shown_up;
> };
> #endif
> #endif
> diff --git a/migration/ram.c b/migration/ram.c
> index 67ca3d5d51..f29faa82d6 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2362,6 +2362,10 @@ static void ram_bitmaps_destroy(void)
> block->bmap = NULL;
> g_free(block->file_bmap);
> block->file_bmap = NULL;
> + g_free(block->shadow_bmap);
> + block->shadow_bmap = NULL;
> + g_free(block->iter_bmap);
> + block->iter_bmap = NULL;
> }
> }
>
> @@ -2753,6 +2757,8 @@ static void ram_list_init_bitmaps(void)
> }
> block->clear_bmap_shift = shift;
> block->clear_bmap = bitmap_new(clear_bmap_size(pages, shift));
> + block->shadow_bmap = bitmap_new(pages);
> + block->iter_bmap = bitmap_new(pages);
> }
> }
> }
