On Fri, Sep 27, 2024 at 10:50:01AM +0800, Yong Huang wrote:
> On Fri, Sep 27, 2024 at 3:55 AM Peter Xu <peterx@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 <peterx@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