On Fri, Sep 20, 2024 at 2:45 AM Peter Xu <pet...@redhat.com> wrote:
> 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 > You mean we could do the background sync in the migration thread or in the main thread( eg, using a timer) ? > 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 > > -- Best regards
