Re: [PATCH v1 1/7] migration: Introduce structs for background sync

[Fabiano Rosas <farosas@suse.de>]

Hyman Huang <yong.huang@smartx.com> 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 <yong.huang@smartx.com>
> ---
>  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);
>          }
>      }
>  }