| Message ID | 531750c8d7b6c09f877b5f335a60fab402c168be.1726390098.git.yong.huang@smartx.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | migration: auto-converge refinements for huge VM | expand |
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); > } > } > }
On Tue, Sep 17, 2024 at 5:11 AM Fabiano Rosas <farosas@suse.de> wrote: > 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. > Using the traditional sync method, the page sending logic iterates the "bmap" to transfer dirty pages while the CPU throttle logic counts the amount of new dirty pages and detects convergence. There are two uses for "bmap". Using the modern sync method, "bmap" is used for transfer dirty pages and "iter_bmap" is used to track new dirty pages. > I'm not sure I follow what "in turn" is supposed to mean in this > sentence. Could you clarify? > Here I want to express "in sequence". But failed obviously. :( > > > + */ > > +#define RAMBLOCK_SYN_LEGACY_ITER (1U << 0) > > So ITER is as opposed to background? I'm a bit confused with the terms. > Yes. > > > + > > +/* > > + * 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. > The bitmap they utilized to track the dirty page information was the distinction between the "legacy iteration" and the "modern iteration." The "iter_bmap" field is used by the "modern iteration" while the "bmap" field is used by the "legacy iteration." Since the refinement is now transparent and there is no API available to change the sync method, I actually want to remove it right now in order to simplify the logic. I'll include it in the next version. > > > + > > +#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 we drop the "legacy modern", we can simplify the following logic too. > + 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); > > } > > } > > } >
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. + */ +#define RAMBLOCK_SYN_LEGACY_ITER (1U << 0) + +/* + * 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) + +#define RAMBLOCK_SYN_MASK (0x7) + +typedef enum RAMBlockSynMode { + RAMBLOCK_SYN_LEGACY, /* Old-fashined mode */ + RAMBLOCK_SYN_MODERN, /* Background-sync-supported mode */ +} RAMBlockSynMode; + 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); } } }
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(+)