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. > The original logic will update the bmap for each sync, which was used to conduct the dirty page sending. In the background sync logic, we do not want to update bmap to interfere with the behavior of page sending for each background sync, since the bitmap we are syncing is only used to detect the convergence and do the CPU throttle. The iteration sync wants to 1: sync dirty bitmap, 2:detect convergence, 3: do the CPU throttle and 4: use the bmap fetched to conduct the page sending, while the background sync only does the 1,2,3. They have different purposes. These logic need at least two bitmap, one is used to page sending and another is used for CPU throttling, to achieve this, we introduced the iter_bmap as the temporary bitmap to store the dirty page information during background sync and copy it to the bmap in the iteration sync logic. However, the dirty page information in iter_bmap may be repetitive since the dirty pages it records could be sent after background syncing, we introduced the shadow_bmap to help calculate the dirty pages having been sent during two background syncs. > However I wonder whether that can be achieved in a simpler manner by > I have tried my best to make the solution simpler but failed. :( > e.g. invoke migration_bitmap_sync_precopy() more frequently during > Yes, invoke migration_bitmap_sync_precopy more frequently is also my first idea but it involves bitmap updating and interfere with the behavior of page sending, it also affects the migration information stats and interfere other migration logic such as migration_update_rates(). > 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 > > Thanks, Yong -- Best regards
