On Tue, Sep 17, 2024 at 02:48:03PM +0800, Yong Huang wrote: > On Tue, Sep 17, 2024 at 5:11 AM Fabiano Rosas <faro...@suse.de> wrote: > > > Hyman Huang <yong.hu...@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.hu...@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.
How confident do we think the new way is better than the old? If it'll be 100% / always better, I agree we can consider removing the old. But is it always better? At least it consumes much more resources.. Otherwise, we can still leave that logic as-is but use a migration property to turn it on only on new machines I think. Besides, could you explain why the solution needs to be this complex? My previous question was that we sync dirty too less, while auto converge relies on dirty information, so that means auto converge can be adjusted too unfrequently. However I wonder whether that can be achieved in a simpler manner by e.g. invoke migration_bitmap_sync_precopy() more frequently during migration, for example, in ram_save_iterate() - not every time but the iterate() is invoked much more frequent, and maybe we can do sync from time to time. I also don't see why we need a separate thread, plus two new bitmaps, to achieve this.. I didn't read in-depth yet, but I thought dirty sync requires bql anyway, then I don't yet understand why the two bitmaps are required. If the bitmaps are introduced in the 1st patch, IMO it'll be great to explain clearly on why they're needed here. Thanks, -- Peter Xu
