On Fri, Sep 27, 2024 at 11:35 PM Peter Xu <pet...@redhat.com> wrote:
> On Fri, Sep 27, 2024 at 10:50:01AM +0800, Yong Huang wrote: > > On Fri, Sep 27, 2024 at 3:55 AM Peter Xu <pet...@redhat.com> wrote: > > > > > On Fri, Sep 27, 2024 at 02:13:47AM +0800, Yong Huang wrote: > > > > On Thu, Sep 26, 2024 at 3:17 AM Peter Xu <pet...@redhat.com> wrote: > > > > > > > > > On Fri, Sep 20, 2024 at 10:43:31AM +0800, Yong Huang wrote: > > > > > > 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(). > > > > > > > > > > Could you elaborate? > > > > > > > > > > For example, what happens if we start to sync in > ram_save_iterate() for > > > > > some time intervals (e.g. 5 seconds)? > > > > > > > > > > > > > I didn't try to sync in ram_save_iterate but in the > > > > migration_bitmap_sync_precopy. > > > > > > > > If we use the migration_bitmap_sync_precopy in the ram_save_iterate > > > > function, > > > > This approach seems to be correct. However, the bitmap will be > updated as > > > > the > > > > migration thread iterates through each dirty page in the RAMBlock > list. > > > > Compared > > > > to the existing implementation, this is different but still > > > straightforward; > > > > I'll give it a shot soon to see if it works. > > > > > > It's still serialized in the migration thread, so I'd expect it is > similar > > > > > > > What does "serialized" mean? > > I meant sync() never happens before concurrently with RAM pages being > iterated, simply because sync() previously only happens in the migration > thread, which is still the same thread that initiate the movement of pages. > > > > > How about we: > > 1. invoke the migration_bitmap_sync_precopy in a timer(bg_sync_timer) > hook, > > every 5 seconds. > > 2. register the bg_sync_timer in the main loop when the machine starts > like > > throttle_timer > > 3. activate the timer when ram_save_iterate gets called and deactivate > it in > > the ram_save_cleanup gracefully during migration. > > > > I think it is simple enough and also isn't "serialized"? > > If you want to do that with timer that's ok, but then IIUC it doesn't need > to involve ram.c code at all. > The timer hook will call the migration_bitmap_sync_precopy() which is implemented in ram.c, maybe we can define the hook function in ram.c and expose it in ram.h? > > You can rely on cpu_throttle_get_percentage() too just like the throttle > timer, and it'll work naturally with migration because outside migration > the throttle will be cleared (cpu_throttle_stop() at finish/fail/cancel..). > Relying on cpu_throttle_get_percentage() may miss the sync time window during the second iteration when it last a long time while the throtlle hasn't started yet. I'll think through your idea and apply it as possible. > > Then it also gracefully align the async thread sync() that it only happens > with auto-converge is enabled. Yeh that may look better.. and stick the > code together with cpu-throttle.c seems nice. > > Side note: one thing regarind to sync() is ram_init_bitmaps() sync once, > while I don't see why it's necessary. I remember I tried to remove it but > maybe I hit some issues and I didn't dig further. If you're working on > sync() anyway not sure whether you'd like to have a look. > > -- > Peter Xu > > Thanks, Yong -- Best regards
