Re: [PATCH v1 1/7] migration: Introduce structs for background sync

[Hyman Huang <yong.huang@smartx.com>]

在 2024/9/27 23:35, Peter Xu 写道:
> 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.
>
> 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.

Ok, Thanks for the advices, i'll check it and see how it goes.

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

Agree, I'll try it after working out current series.


Yong