[PATCH v4 0/2] Some modification about ram_save_host_page()

[PATCH v4 0/2] Some modification about ram_save_host_page()

[Kunkun Jiang]

Hi all,

This series include patches as below:
Patch 1:
- reduce unnecessary rate limiting in ram_save_host_page()

Patch 2:
- optimized ram_save_host_page() by using migration_bitmap_find_dirty() to find
dirty pages

History:

v3 -> v4:
- Remove the modification to ram_save_host_page() comment [Peter Xu]
- Remove the renaming of tmppages 

v2 -> v3:
- Reduce unnecessary rate limiting if nothing is sent in the current iteration [David Edmondson]
- Invert the body of the loop in ram_save_host_page() [David Edmondson]

v1 -> v2:
- Modify ram_save_host_page() comment [David Edmondson]
- Remove 'goto' [David Edmondson]

Kunkun Jiang (2):
  migration/ram: Reduce unnecessary rate limiting
  migration/ram: Optimize ram_save_host_page()

 migration/ram.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

-- 
2.23.0

[PATCH v4 1/2] migration/ram: Reduce unnecessary rate limiting

[Kunkun Jiang]

When the host page is a huge page and something is sent in the
current iteration, migration_rate_limit() should be executed.
If not, it can be omitted.

Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
Reviewed-by: David Edmondson <david.edmondson@oracle.com>
---
 migration/ram.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 72143da0ac..3eb5b0d7a7 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2015,8 +2015,13 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
 
         pages += tmppages;
         pss->page++;
-        /* Allow rate limiting to happen in the middle of huge pages */
-        migration_rate_limit();
+        /*
+         * Allow rate limiting to happen in the middle of huge pages if
+         * something is sent in the current iteration.
+         */
+        if (pagesize_bits > 1 && tmppages > 0) {
+            migration_rate_limit();
+        }
     } while ((pss->page & (pagesize_bits - 1)) &&
              offset_in_ramblock(pss->block,
                                 ((ram_addr_t)pss->page) << TARGET_PAGE_BITS));
-- 
2.23.0

Re: [PATCH v4 1/2] migration/ram: Reduce unnecessary rate limiting

[Peter Xu]

On Tue, Mar 16, 2021 at 08:57:15PM +0800, Kunkun Jiang wrote:
> When the host page is a huge page and something is sent in the
> current iteration, migration_rate_limit() should be executed.
> If not, it can be omitted.
> 
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
> ---
>  migration/ram.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 72143da0ac..3eb5b0d7a7 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2015,8 +2015,13 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>  
>          pages += tmppages;
>          pss->page++;
> -        /* Allow rate limiting to happen in the middle of huge pages */
> -        migration_rate_limit();
> +        /*
> +         * Allow rate limiting to happen in the middle of huge pages if
> +         * something is sent in the current iteration.
> +         */
> +        if (pagesize_bits > 1 && tmppages > 0) {
> +            migration_rate_limit();
> +        }

Sorry I'm still not a fan of this - I'd even prefer calling that once more just
to make sure it won't be forgotten to be called..  Not to say it's merely a noop.

I'll leave this to Dave..  Maybe I'm too harsh! :)

-- 
Peter Xu

Re: [PATCH v4 1/2] migration/ram: Reduce unnecessary rate limiting

[Kunkun Jiang]

Hi  Peter,

On 2021/3/17 5:39, Peter Xu wrote:
> On Tue, Mar 16, 2021 at 08:57:15PM +0800, Kunkun Jiang wrote:
>> When the host page is a huge page and something is sent in the
>> current iteration, migration_rate_limit() should be executed.
>> If not, it can be omitted.
>>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
>> ---
>>   migration/ram.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 72143da0ac..3eb5b0d7a7 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2015,8 +2015,13 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>>   
>>           pages += tmppages;
>>           pss->page++;
>> -        /* Allow rate limiting to happen in the middle of huge pages */
>> -        migration_rate_limit();
>> +        /*
>> +         * Allow rate limiting to happen in the middle of huge pages if
>> +         * something is sent in the current iteration.
>> +         */
>> +        if (pagesize_bits > 1 && tmppages > 0) {
>> +            migration_rate_limit();
>> +        }
> Sorry I'm still not a fan of this - I'd even prefer calling that once more just
> to make sure it won't be forgotten to be called..  Not to say it's merely a noop.
>
> I'll leave this to Dave..  Maybe I'm too harsh! :)
>
You are very serious and meticulous. I like your character very much.😉
This patch was used to reviewed by David. So, I want to know what
his opinion is.

@David
Hi David, what is your opinion on this patch?

Thanks,
Kunkun Jiang

Re: [PATCH v4 1/2] migration/ram: Reduce unnecessary rate limiting

[David Edmondson]

On Wednesday, 2021-03-17 at 09:37:11 +08, Kunkun Jiang wrote:

> Hi  Peter,
>
> On 2021/3/17 5:39, Peter Xu wrote:
>> On Tue, Mar 16, 2021 at 08:57:15PM +0800, Kunkun Jiang wrote:
>>> When the host page is a huge page and something is sent in the
>>> current iteration, migration_rate_limit() should be executed.
>>> If not, it can be omitted.
>>>
>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>>> Reviewed-by: David Edmondson <david.edmondson@oracle.com>
>>> ---
>>>   migration/ram.c | 9 +++++++--
>>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/migration/ram.c b/migration/ram.c
>>> index 72143da0ac..3eb5b0d7a7 100644
>>> --- a/migration/ram.c
>>> +++ b/migration/ram.c
>>> @@ -2015,8 +2015,13 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>>>   
>>>           pages += tmppages;
>>>           pss->page++;
>>> -        /* Allow rate limiting to happen in the middle of huge pages */
>>> -        migration_rate_limit();
>>> +        /*
>>> +         * Allow rate limiting to happen in the middle of huge pages if
>>> +         * something is sent in the current iteration.
>>> +         */
>>> +        if (pagesize_bits > 1 && tmppages > 0) {
>>> +            migration_rate_limit();
>>> +        }
>> Sorry I'm still not a fan of this - I'd even prefer calling that once more just
>> to make sure it won't be forgotten to be called..  Not to say it's merely a noop.
>>
>> I'll leave this to Dave..  Maybe I'm too harsh! :)
>>
> You are very serious and meticulous. I like your character very much.😉
> This patch was used to reviewed by David. So, I want to know what
> his opinion is.
>
> @David
> Hi David, what is your opinion on this patch?

I suspect that this referred to David Gilbert rather than me :-)

> Thanks,
> Kunkun Jiang

dme.
-- 
Time is waiting to explain, why refuse?

Re: [PATCH v4 1/2] migration/ram: Reduce unnecessary rate limiting

[Dr. David Alan Gilbert]

* Kunkun Jiang (jiangkunkun@huawei.com) wrote:
> Hi  Peter,
> 
> On 2021/3/17 5:39, Peter Xu wrote:
> > On Tue, Mar 16, 2021 at 08:57:15PM +0800, Kunkun Jiang wrote:
> > > When the host page is a huge page and something is sent in the
> > > current iteration, migration_rate_limit() should be executed.
> > > If not, it can be omitted.
> > > 
> > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > > Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> > > Reviewed-by: David Edmondson <david.edmondson@oracle.com>
> > > ---
> > >   migration/ram.c | 9 +++++++--
> > >   1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 72143da0ac..3eb5b0d7a7 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -2015,8 +2015,13 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
> > >           pages += tmppages;
> > >           pss->page++;
> > > -        /* Allow rate limiting to happen in the middle of huge pages */
> > > -        migration_rate_limit();
> > > +        /*
> > > +         * Allow rate limiting to happen in the middle of huge pages if
> > > +         * something is sent in the current iteration.
> > > +         */
> > > +        if (pagesize_bits > 1 && tmppages > 0) {
> > > +            migration_rate_limit();
> > > +        }
> > Sorry I'm still not a fan of this - I'd even prefer calling that once more just
> > to make sure it won't be forgotten to be called..  Not to say it's merely a noop.
> > 
> > I'll leave this to Dave..  Maybe I'm too harsh! :)
> > 
> You are very serious and meticulous. I like your character very much.😉
> This patch was used to reviewed by David. So, I want to know what
> his opinion is.
> 
> @David
> Hi David, what is your opinion on this patch?

Yes, I think this is OK;
  a) The qemu_file_rate_limit in the loop in ram_save_iterate still
     gets called, so that covers the 'pagesize_bits > 1' part of the if
  b) As soon as we send any part of the hugepage the 'tmppages > 0'
     triggers and we get the check back again.

So I guess this mostly helps the case where we have big huge pages
which are mostly not-dirty, and we spend a lot of time in
ram_save_host_page searching for the target page to send.



Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> Thanks,
> Kunkun Jiang
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH v4 2/2] migration/ram: Optimize ram_save_host_page()

[Kunkun Jiang]

Starting from pss->page, ram_save_host_page() will check every page
and send the dirty pages up to the end of the current host page or
the boundary of used_length of the block. If the host page size is
a huge page, the step "check" will take a lot of time.

It will improve performance to use migration_bitmap_find_dirty().

Tested on Kunpeng 920; VM parameters: 1U 4G (page size 1G)
The time of ram_save_host_page() in the last round of ram saving:
before optimize: 9250us		after optimize: 34us

Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
---
 migration/ram.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 3eb5b0d7a7..1d9c24aa85 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1993,6 +1993,8 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
     int tmppages, pages = 0;
     size_t pagesize_bits =
         qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
+    unsigned long hostpage_boundary =
+        QEMU_ALIGN_UP(pss->page + 1, pagesize_bits);
     unsigned long start_page = pss->page;
     int res;
 
@@ -2003,30 +2005,27 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
 
     do {
         /* Check the pages is dirty and if it is send it */
-        if (!migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
-            pss->page++;
-            continue;
-        }
-
-        tmppages = ram_save_target_page(rs, pss, last_stage);
-        if (tmppages < 0) {
-            return tmppages;
-        }
+        if (migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
+            tmppages = ram_save_target_page(rs, pss, last_stage);
+            if (tmppages < 0) {
+                return tmppages;
+            }
 
-        pages += tmppages;
-        pss->page++;
-        /*
-         * Allow rate limiting to happen in the middle of huge pages if
-         * something is sent in the current iteration.
-         */
-        if (pagesize_bits > 1 && tmppages > 0) {
-            migration_rate_limit();
+            pages += tmppages;
+            /*
+             * Allow rate limiting to happen in the middle of huge pages if
+             * something is sent in the current iteration.
+             */
+            if (pagesize_bits > 1 && tmppages > 0) {
+                migration_rate_limit();
+            }
         }
-    } while ((pss->page & (pagesize_bits - 1)) &&
+        pss->page = migration_bitmap_find_dirty(rs, pss->block, pss->page);
+    } while ((pss->page < hostpage_boundary) &&
              offset_in_ramblock(pss->block,
                                 ((ram_addr_t)pss->page) << TARGET_PAGE_BITS));
-    /* The offset we leave with is the last one we looked at */
-    pss->page--;
+    /* The offset we leave with is the min boundary of host page and block */
+    pss->page = MIN(pss->page, hostpage_boundary) - 1;
 
     res = ram_save_release_protection(rs, pss, start_page);
     return (res < 0 ? res : pages);
-- 
2.23.0

Re: [PATCH v4 2/2] migration/ram: Optimize ram_save_host_page()

[Peter Xu]

On Tue, Mar 16, 2021 at 08:57:16PM +0800, Kunkun Jiang wrote:
> Starting from pss->page, ram_save_host_page() will check every page
> and send the dirty pages up to the end of the current host page or
> the boundary of used_length of the block. If the host page size is
> a huge page, the step "check" will take a lot of time.
> 
> It will improve performance to use migration_bitmap_find_dirty().
> 
> Tested on Kunpeng 920; VM parameters: 1U 4G (page size 1G)
> The time of ram_save_host_page() in the last round of ram saving:
> before optimize: 9250us		after optimize: 34us
> 
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

Re: [PATCH v4 0/2] Some modification about ram_save_host_page()

[Kunkun Jiang]

Kindly ping,

Hi David Alan Gilbert,
Will this series be picked up soon, or is there any other work for me to do?

Best Regards,
Kunkun Jiang

On 2021/3/16 20:57, Kunkun Jiang wrote:
> Hi all,
>
> This series include patches as below:
> Patch 1:
> - reduce unnecessary rate limiting in ram_save_host_page()
>
> Patch 2:
> - optimized ram_save_host_page() by using migration_bitmap_find_dirty() to find
> dirty pages
>
> History:
>
> v3 -> v4:
> - Remove the modification to ram_save_host_page() comment [Peter Xu]
> - Remove the renaming of tmppages
>
> v2 -> v3:
> - Reduce unnecessary rate limiting if nothing is sent in the current iteration [David Edmondson]
> - Invert the body of the loop in ram_save_host_page() [David Edmondson]
>
> v1 -> v2:
> - Modify ram_save_host_page() comment [David Edmondson]
> - Remove 'goto' [David Edmondson]
>
> Kunkun Jiang (2):
>    migration/ram: Reduce unnecessary rate limiting
>    migration/ram: Optimize ram_save_host_page()
>
>   migration/ram.c | 34 +++++++++++++++++++---------------
>   1 file changed, 19 insertions(+), 15 deletions(-)
>

Re: [PATCH v4 0/2] Some modification about ram_save_host_page()

[Dr. David Alan Gilbert]

* Kunkun Jiang (jiangkunkun@huawei.com) wrote:
> Kindly ping,
> 
> Hi David Alan Gilbert,
> Will this series be picked up soon, or is there any other work for me to do?

You don't need to do anything, but it did miss the cutoff for soft
freeze; since it's an optimisation not a fix; it's still on my list so
it'll go in just as soon as 6.1 opens up.

Dave

> Best Regards,
> Kunkun Jiang
> 
> On 2021/3/16 20:57, Kunkun Jiang wrote:
> > Hi all,
> > 
> > This series include patches as below:
> > Patch 1:
> > - reduce unnecessary rate limiting in ram_save_host_page()
> > 
> > Patch 2:
> > - optimized ram_save_host_page() by using migration_bitmap_find_dirty() to find
> > dirty pages
> > 
> > History:
> > 
> > v3 -> v4:
> > - Remove the modification to ram_save_host_page() comment [Peter Xu]
> > - Remove the renaming of tmppages
> > 
> > v2 -> v3:
> > - Reduce unnecessary rate limiting if nothing is sent in the current iteration [David Edmondson]
> > - Invert the body of the loop in ram_save_host_page() [David Edmondson]
> > 
> > v1 -> v2:
> > - Modify ram_save_host_page() comment [David Edmondson]
> > - Remove 'goto' [David Edmondson]
> > 
> > Kunkun Jiang (2):
> >    migration/ram: Reduce unnecessary rate limiting
> >    migration/ram: Optimize ram_save_host_page()
> > 
> >   migration/ram.c | 34 +++++++++++++++++++---------------
> >   1 file changed, 19 insertions(+), 15 deletions(-)
> > 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v4 0/2] Some modification about ram_save_host_page()

[Kunkun Jiang]

Hi Dave,

On 2021/4/7 1:14, Dr. David Alan Gilbert wrote:
> * Kunkun Jiang (jiangkunkun@huawei.com) wrote:
>> Kindly ping,
>>
>> Hi David Alan Gilbert,
>> Will this series be picked up soon, or is there any other work for me to do?
> You don't need to do anything, but it did miss the cutoff for soft
> freeze; since it's an optimisation not a fix; it's still on my list so
> it'll go in just as soon as 6.1 opens up.
>
> Dave
Okay, I see. Thank you.

Best Regards,
Kunkun Jiang
>> Best Regards,
>> Kunkun Jiang
>>
>> On 2021/3/16 20:57, Kunkun Jiang wrote:
>>> Hi all,
>>>
>>> This series include patches as below:
>>> Patch 1:
>>> - reduce unnecessary rate limiting in ram_save_host_page()
>>>
>>> Patch 2:
>>> - optimized ram_save_host_page() by using migration_bitmap_find_dirty() to find
>>> dirty pages
>>>
>>> History:
>>>
>>> v3 -> v4:
>>> - Remove the modification to ram_save_host_page() comment [Peter Xu]
>>> - Remove the renaming of tmppages
>>>
>>> v2 -> v3:
>>> - Reduce unnecessary rate limiting if nothing is sent in the current iteration [David Edmondson]
>>> - Invert the body of the loop in ram_save_host_page() [David Edmondson]
>>>
>>> v1 -> v2:
>>> - Modify ram_save_host_page() comment [David Edmondson]
>>> - Remove 'goto' [David Edmondson]
>>>
>>> Kunkun Jiang (2):
>>>     migration/ram: Reduce unnecessary rate limiting
>>>     migration/ram: Optimize ram_save_host_page()
>>>
>>>    migration/ram.c | 34 +++++++++++++++++++---------------
>>>    1 file changed, 19 insertions(+), 15 deletions(-)
>>>

Re: [PATCH v4 0/2] Some modification about ram_save_host_page()

[Dr. David Alan Gilbert]

* Kunkun Jiang (jiangkunkun@huawei.com) wrote:
> Hi all,

Queued; sorry for the delay

> This series include patches as below:
> Patch 1:
> - reduce unnecessary rate limiting in ram_save_host_page()
> 
> Patch 2:
> - optimized ram_save_host_page() by using migration_bitmap_find_dirty() to find
> dirty pages
> 
> History:
> 
> v3 -> v4:
> - Remove the modification to ram_save_host_page() comment [Peter Xu]
> - Remove the renaming of tmppages 
> 
> v2 -> v3:
> - Reduce unnecessary rate limiting if nothing is sent in the current iteration [David Edmondson]
> - Invert the body of the loop in ram_save_host_page() [David Edmondson]
> 
> v1 -> v2:
> - Modify ram_save_host_page() comment [David Edmondson]
> - Remove 'goto' [David Edmondson]
> 
> Kunkun Jiang (2):
>   migration/ram: Reduce unnecessary rate limiting
>   migration/ram: Optimize ram_save_host_page()
> 
>  migration/ram.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> -- 
> 2.23.0
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK