Re: [RFC PATCH] migration/ram: avoid to do log clear in the last round

[Yanfei Xu <isyanfei.xu@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3813 bytes --]


On 2025/5/21 04:33, Peter Xu wrote:
> On Tue, May 20, 2025 at 04:05:57PM -0300, Fabiano Rosas wrote:
>> Yanfei Xu<yanfei.xu@bytedance.com> writes:
>>
>>> There won't be any ram sync after the stage of save_complete, therefore
>>> it's unnecessary to do manually protect for dirty pages being sent. Skip
>>> to do this in last round can reduce noticeable downtime.
>>>
>>> Signed-off-by: Yanfei Xu<yanfei.xu@bytedance.com>
>>> ---
>>> As I don't have proper machine to test this patch in qemu and verify if it has
>>> risks like in postcopy, colo and so on.(But I tested this idea on my rust VMM,
>>> it works and can reduce ~50ms for a 128GB guest). So I raise the patch with RFC
>>> for suggestions.
>>>
>>>   migration/ram.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index e12913b43e..2b169cdd18 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -838,7 +838,9 @@ static inline bool migration_bitmap_clear_dirty(RAMState *rs,
>>>        * the page in the chunk we clear the remote dirty bitmap for all.
>>>        * Clearing it earlier won't be a problem, but too late will.
>>>        */
>>> -    migration_clear_memory_region_dirty_bitmap(rb, page);
>>> +    if (!rs->last_stage) {
>>> +        migration_clear_memory_region_dirty_bitmap(rb, page);
>>> +    }
>>>   
>>>       ret = test_and_clear_bit(page, rb->bmap);
>>>       if (ret) {
>> Well, it looks ok to me and passes all my tests.
>>
>> Tested-by: Fabiano Rosas<farosas@suse.de>
>> Reviewed-by: Fabiano Rosas<farosas@suse.de>
> Thanks both!
>
> I plan to test a bit on this patch later to see how much perf we can get in
> QEMU.  Since it makes perfect sense on its own, queued it for now, and the
> plan is with below comments squashed in.  Let me know if there's comments.
>
> Postcopy unfortunately still cannot benefit much from this change, but I'll
> prepare some patches soon so that this should ideally also work for the
> whole lifecycle of postcopy.  After that done, I am expecting some further
> page fault latency reduction with this change.

Thanks for Fabiano's test and Peter's elaboration in comments.

Look foward to the performance data.

Regards, Yanfei

> diff --git a/migration/ram.c b/migration/ram.c
> index db70699f95..fd8d83b63c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -831,14 +831,20 @@ static inline bool migration_bitmap_clear_dirty(RAMState *rs,
>       bool ret;
>   
>       /*
> -     * Clear dirty bitmap if needed.  This _must_ be called before we
> -     * send any of the page in the chunk because we need to make sure
> -     * we can capture further page content changes when we sync dirty
> -     * log the next time.  So as long as we are going to send any of
> -     * the page in the chunk we clear the remote dirty bitmap for all.
> -     * Clearing it earlier won't be a problem, but too late will.
> +     * During the last stage (after source VM stopped), resetting the write
> +     * protections isn't needed as we know there will be either (1) no
> +     * further writes if migration will complete, or (2) migration fails
> +     * at last then tracking isn't needed either.
>        */
>       if (!rs->last_stage) {
> +        /*
> +         * Clear dirty bitmap if needed.  This _must_ be called before we
> +         * send any of the page in the chunk because we need to make sure
> +         * we can capture further page content changes when we sync dirty
> +         * log the next time.  So as long as we are going to send any of
> +         * the page in the chunk we clear the remote dirty bitmap for all.
> +         * Clearing it earlier won't be a problem, but too late will.
> +         */
>           migration_clear_memory_region_dirty_bitmap(rb, page);
>       }
>

[-- Attachment #2: Type: text/html, Size: 4950 bytes --]