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. 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..). 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
