[PATCH v3 0/3] Some modifications about ram_save_host_page()

[PATCH v3 0/3] Some modifications about ram_save_host_page()

[Kunkun Jiang]

Hi,

This series include patches as below:
Patch 1:
- modified the comment ram_save_host_page() to make them match each other

Patch 2:
- reduce unnecessary rate limitting in ram_save_host_page()

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

History:

v2 -> v3:
- Reduce unnecessary rate limitting 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 (3):
  migration/ram: Modify the code comment of ram_save_host_page()
  migration/ram: Reduce unnecessary rate limiting
  migration/ram: Optimize ram_save_host_page()

 migration/ram.c | 54 ++++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

-- 
2.23.0

[PATCH v3 1/3] migration/ram: Modify the code comment of ram_save_host_page()

[Kunkun Jiang]

The ram_save_host_page() has been modified several times
since its birth. But the comment hasn't been modified as it should
be. It'd better to modify the comment to explain ram_save_host_page()
more clearly.

Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
---
 migration/ram.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 72143da0ac..a168da5cdd 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1970,15 +1970,13 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
 }
 
 /**
- * ram_save_host_page: save a whole host page
+ * ram_save_host_page: save a whole host page or the rest of a RAMBlock
  *
- * Starting at *offset send pages up to the end of the current host
- * page. It's valid for the initial offset to point into the middle of
- * a host page in which case the remainder of the hostpage is sent.
- * Only dirty target pages are sent. Note that the host page size may
- * be a huge page for this block.
- * The saving stops at the boundary of the used_length of the block
- * if the RAMBlock isn't a multiple of the host page size.
+ * Send dirty pages between pss->page and either the end of that page
+ * or the used_length of the RAMBlock, whichever is smaller.
+ *
+ * Note that if the host page is a huge page, pss->page may be in the
+ * middle of that page.
  *
  * Returns the number of pages written or negative on error
  *
@@ -2002,7 +2000,7 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
     }
 
     do {
-        /* Check the pages is dirty and if it is send it */
+        /* Check if the page is dirty and send it if it is */
         if (!migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
             pss->page++;
             continue;
-- 
2.23.0

[PATCH v3 2/3] migration/ram: Reduce unnecessary rate limiting

[Kunkun Jiang]

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

Rename tmppages to pages_this_iteration to express its meaning
more clearly.

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

diff --git a/migration/ram.c b/migration/ram.c
index a168da5cdd..9fc5b2997c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1988,7 +1988,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
 static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
                               bool last_stage)
 {
-    int tmppages, pages = 0;
+    int pages = 0;
     size_t pagesize_bits =
         qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
     unsigned long start_page = pss->page;
@@ -2000,21 +2000,28 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
     }
 
     do {
+        int pages_this_iteration = 0;
+
         /* Check if the page is dirty and send it if it is */
         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;
+        pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
+        if (pages_this_iteration < 0) {
+            return pages_this_iteration;
         }
 
-        pages += tmppages;
+        pages += pages_this_iteration;
         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 && pages_this_iteration > 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

[PATCH v3 3/3] 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.

This will improve performance to use migration_bitmap_find_dirty().

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 9fc5b2997c..28215aefe4 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1991,6 +1991,8 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
     int 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,
         int pages_this_iteration = 0;
 
         /* Check if the page is dirty and send it if it is */
-        if (!migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
-            pss->page++;
-            continue;
-        }
-
-        pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
-        if (pages_this_iteration < 0) {
-            return pages_this_iteration;
-        }
+        if (migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
+            pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
+            if (pages_this_iteration < 0) {
+                return pages_this_iteration;
+            }
 
-        pages += pages_this_iteration;
-        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 && pages_this_iteration > 0) {
-            migration_rate_limit();
+            pages += pages_this_iteration;
+            /*
+             * Allow rate limiting to happen in the middle of huge pages if
+             * something is sent in the current iteration.
+             */
+            if (pagesize_bits > 1 && pages_this_iteration > 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 v3 1/3] migration/ram: Modify the code comment of ram_save_host_page()

[Peter Xu]

On Fri, Mar 05, 2021 at 03:50:33PM +0800, Kunkun Jiang wrote:
> The ram_save_host_page() has been modified several times
> since its birth. But the comment hasn't been modified as it should
> be. It'd better to modify the comment to explain ram_save_host_page()
> more clearly.
> 
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> ---
>  migration/ram.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 72143da0ac..a168da5cdd 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1970,15 +1970,13 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>  }
>  
>  /**
> - * ram_save_host_page: save a whole host page
> + * ram_save_host_page: save a whole host page or the rest of a RAMBlock
>   *
> - * Starting at *offset send pages up to the end of the current host
> - * page. It's valid for the initial offset to point into the middle of
> - * a host page in which case the remainder of the hostpage is sent.
> - * Only dirty target pages are sent. Note that the host page size may
> - * be a huge page for this block.
> - * The saving stops at the boundary of the used_length of the block
> - * if the RAMBlock isn't a multiple of the host page size.
> + * Send dirty pages between pss->page and either the end of that page
> + * or the used_length of the RAMBlock, whichever is smaller.
> + *
> + * Note that if the host page is a huge page, pss->page may be in the
> + * middle of that page.

It reads more like a shorter version of original comment..  Could you point out
what's the major difference?  Since the original comment looks still good to me.

>   *
>   * Returns the number of pages written or negative on error
>   *
> @@ -2002,7 +2000,7 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>      }
>  
>      do {
> -        /* Check the pages is dirty and if it is send it */
> +        /* Check if the page is dirty and send it if it is */

This line fixes some English issues, it seems.  Looks okay, but normally I
won't change it unless the wording was too weird, so as to keep the git record
or the original lines.  Here the original looks still okay, no strong opinion.

Thanks,

>          if (!migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
>              pss->page++;
>              continue;
> -- 
> 2.23.0
> 

-- 
Peter Xu

Re: [PATCH v3 2/3] migration/ram: Reduce unnecessary rate limiting

[Peter Xu]

Kunkun,

On Fri, Mar 05, 2021 at 03:50:34PM +0800, Kunkun Jiang wrote:
> When the host page is a huge page and something is sent in the
> current iteration, the migration_rate_limit() should be executed.
> If not, this function can be omitted to save time.
> 
> Rename tmppages to pages_this_iteration to express its meaning
> more clearly.
> 
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> ---
>  migration/ram.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index a168da5cdd..9fc5b2997c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1988,7 +1988,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>  static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>                                bool last_stage)
>  {
> -    int tmppages, pages = 0;
> +    int pages = 0;
>      size_t pagesize_bits =
>          qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
>      unsigned long start_page = pss->page;
> @@ -2000,21 +2000,28 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>      }
>  
>      do {
> +        int pages_this_iteration = 0;
> +
>          /* Check if the page is dirty and send it if it is */
>          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;
> +        pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
> +        if (pages_this_iteration < 0) {
> +            return pages_this_iteration;
>          }
>  
> -        pages += tmppages;
> +        pages += pages_this_iteration;

To me, both names are okay, it's just that the new name doesn't really provide
a lot more new information, while it's even longer...

Since you seem to prefer cleaning up tmppages, I'm actually thinking whether
it should be called as "pages" at all since ram_save_target_page() majorly only
returns either 1 if succeeded or <0 if error.  There's only one very corner
case of xbzrle where it can return 0 in save_xbzrle_page():

    if (encoded_len == 0) {
        trace_save_xbzrle_page_skipping();
        return 0;
    }

I think it means the page didn't change at all, then I'm also wondering maybe
it can also return 1 showing one page migrated (though actually skipped!) which
should still be fine for the callers, e.g., ram_find_and_save_block() who will
finally check this "pages" value.

So I think _maybe_ that's a nicer cleanup to change that "return 0" to "return
1", then another patch to make the return value to be (1) return 0 if page
saved, or (2) return <0 if error.  Then here in ram_save_host_page() tmppages
can be renamed to "ret" or "succeed".

>          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 && pages_this_iteration > 0) {
> +            migration_rate_limit();
> +        }

I don't know whether this matters either..  Two calls in there:

    migration_update_counters(s, now);
    qemu_file_rate_limit(s->to_dst_file);

migration_update_counters() is mostly a noop for 99.9% cases. Looks still okay...

Thanks,

>      } while ((pss->page & (pagesize_bits - 1)) &&
>               offset_in_ramblock(pss->block,
>                                  ((ram_addr_t)pss->page) << TARGET_PAGE_BITS));
> -- 
> 2.23.0
> 

-- 
Peter Xu

Re: [PATCH v3 3/3] migration/ram: Optimize ram_save_host_page()

[Peter Xu]

On Fri, Mar 05, 2021 at 03:50:35PM +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.
> 
> This will improve performance to use migration_bitmap_find_dirty().

Is there any measurement done?

This looks like an optimization, but to me it seems to have changed a lot
context that it doesn't need to... Do you think it'll also work to just look up
dirty again and update pss->page properly if migration_bitmap_clear_dirty()
returned zero?

Thanks,

> 
> 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 9fc5b2997c..28215aefe4 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1991,6 +1991,8 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>      int 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,
>          int pages_this_iteration = 0;
>  
>          /* Check if the page is dirty and send it if it is */
> -        if (!migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
> -            pss->page++;
> -            continue;
> -        }
> -
> -        pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
> -        if (pages_this_iteration < 0) {
> -            return pages_this_iteration;
> -        }
> +        if (migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
> +            pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
> +            if (pages_this_iteration < 0) {
> +                return pages_this_iteration;
> +            }
>  
> -        pages += pages_this_iteration;
> -        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 && pages_this_iteration > 0) {
> -            migration_rate_limit();
> +            pages += pages_this_iteration;
> +            /*
> +             * Allow rate limiting to happen in the middle of huge pages if
> +             * something is sent in the current iteration.
> +             */
> +            if (pagesize_bits > 1 && pages_this_iteration > 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
> 

-- 
Peter Xu

Re: [PATCH v3 1/3] migration/ram: Modify the code comment of ram_save_host_page()

[Kunkun Jiang]

Hi, Peter

On 2021/3/5 21:59, Peter Xu wrote:
> On Fri, Mar 05, 2021 at 03:50:33PM +0800, Kunkun Jiang wrote:
>> The ram_save_host_page() has been modified several times
>> since its birth. But the comment hasn't been modified as it should
>> be. It'd better to modify the comment to explain ram_save_host_page()
>> more clearly.
>>
>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>> ---
>>   migration/ram.c | 16 +++++++---------
>>   1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 72143da0ac..a168da5cdd 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1970,15 +1970,13 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>>   }
>>   
>>   /**
>> - * ram_save_host_page: save a whole host page
>> + * ram_save_host_page: save a whole host page or the rest of a RAMBlock
>>    *
>> - * Starting at *offset send pages up to the end of the current host
>> - * page. It's valid for the initial offset to point into the middle of
>> - * a host page in which case the remainder of the hostpage is sent.
>> - * Only dirty target pages are sent. Note that the host page size may
>> - * be a huge page for this block.
>> - * The saving stops at the boundary of the used_length of the block
>> - * if the RAMBlock isn't a multiple of the host page size.
>> + * Send dirty pages between pss->page and either the end of that page
>> + * or the used_length of the RAMBlock, whichever is smaller.
>> + *
>> + * Note that if the host page is a huge page, pss->page may be in the
>> + * middle of that page.
> It reads more like a shorter version of original comment..  Could you point out
> what's the major difference?  Since the original comment looks still good to me.
Sorry for late reply.
The reason I want to modify the comment is that I think some parts of 
the comment
don't match the code. (1) The brief description of ram_save_host_page() 
missed a
situation that ram_save_host_page() will send dirty pages between 
pass->page and
the used_length of the block, if the RAMBlock isn't a multiple of the 
host page
size. (2) '*offset' should be replaced with pss->page.

So taking the chance of optimizing ram_save_host_page(), I modified the 
comment.
This version comment is suggested by @David Edmondson. Compared with the 
original
comment, I think it looks concise.
>>    *
>>    * Returns the number of pages written or negative on error
>>    *
>> @@ -2002,7 +2000,7 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>>       }
>>   
>>       do {
>> -        /* Check the pages is dirty and if it is send it */
>> +        /* Check if the page is dirty and send it if it is */
> This line fixes some English issues, it seems.  Looks okay, but normally I
> won't change it unless the wording was too weird, so as to keep the git record
> or the original lines.  Here the original looks still okay, no strong opinion.
>
> Thanks,
Yes, the original reads weird to me. Same reason as above, I modified 
this line.

If you think there is no need to modify the original commit, I do not 
insist on
changing it.😁

Thanks,
Kunkun Jiang
>>           if (!migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
>>               pss->page++;
>>               continue;
>> -- 
>> 2.23.0
>>

Re: [PATCH v3 2/3] migration/ram: Reduce unnecessary rate limiting

[Kunkun Jiang]

Hi,

On 2021/3/5 22:22, Peter Xu wrote:
> Kunkun,
>
> On Fri, Mar 05, 2021 at 03:50:34PM +0800, Kunkun Jiang wrote:
>> When the host page is a huge page and something is sent in the
>> current iteration, the migration_rate_limit() should be executed.
>> If not, this function can be omitted to save time.
>>
>> Rename tmppages to pages_this_iteration to express its meaning
>> more clearly.
>>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>> ---
>>   migration/ram.c | 21 ++++++++++++++-------
>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index a168da5cdd..9fc5b2997c 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1988,7 +1988,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>>   static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>>                                 bool last_stage)
>>   {
>> -    int tmppages, pages = 0;
>> +    int pages = 0;
>>       size_t pagesize_bits =
>>           qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
>>       unsigned long start_page = pss->page;
>> @@ -2000,21 +2000,28 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>>       }
>>   
>>       do {
>> +        int pages_this_iteration = 0;
>> +
>>           /* Check if the page is dirty and send it if it is */
>>           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;
>> +        pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
>> +        if (pages_this_iteration < 0) {
>> +            return pages_this_iteration;
>>           }
>>   
>> -        pages += tmppages;
>> +        pages += pages_this_iteration;
> To me, both names are okay, it's just that the new name doesn't really provide
> a lot more new information, while it's even longer...
>
> Since you seem to prefer cleaning up tmppages, I'm actually thinking whether
> it should be called as "pages" at all since ram_save_target_page() majorly only
> returns either 1 if succeeded or <0 if error.  There's only one very corner
> case of xbzrle where it can return 0 in save_xbzrle_page():
>
>      if (encoded_len == 0) {
>          trace_save_xbzrle_page_skipping();
>          return 0;
>      }
>
> I think it means the page didn't change at all, then I'm also wondering maybe
> it can also return 1 showing one page migrated (though actually skipped!) which
> should still be fine for the callers, e.g., ram_find_and_save_block() who will
> finally check this "pages" value.
>
> So I think _maybe_ that's a nicer cleanup to change that "return 0" to "return
> 1", then another patch to make the return value to be (1) return 0 if page
> saved, or (2) return <0 if error.  Then here in ram_save_host_page() tmppages
> can be renamed to "ret" or "succeed".
Thanks for your advice.
change "return 0" to "return 1" would have a slight effect on 
'rs->target_page_count += pages'
in ram_save_iterate(). This may lead to consider more complex 
situations. What do you think of
this?
>>           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 && pages_this_iteration > 0) {
>> +            migration_rate_limit();
>> +        }
> I don't know whether this matters either..  Two calls in there:
>
>      migration_update_counters(s, now);
>      qemu_file_rate_limit(s->to_dst_file);
>
> migration_update_counters() is mostly a noop for 99.9% cases. Looks still okay...
>
> Thanks,
I think even these two calls shouldn't be called if the host page size 
isn't a huge page or
nothing is sent in the current iteration.

Thanks,
Kunkun Jiang
>>       } while ((pss->page & (pagesize_bits - 1)) &&
>>                offset_in_ramblock(pss->block,
>>                                   ((ram_addr_t)pss->page) << TARGET_PAGE_BITS));
>> -- 
>> 2.23.0
>>

Re: [PATCH v3 3/3] migration/ram: Optimize ram_save_host_page()

[Kunkun Jiang]

Hi,

On 2021/3/5 22:30, Peter Xu wrote:
> On Fri, Mar 05, 2021 at 03:50:35PM +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.
>>
>> This will improve performance to use migration_bitmap_find_dirty().
> Is there any measurement done?
I tested it on Kunpeng 920.  VM params: 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
> This looks like an optimization, but to me it seems to have changed a lot
> context that it doesn't need to... Do you think it'll also work to just look up
> dirty again and update pss->page properly if migration_bitmap_clear_dirty()
> returned zero?
>
> Thanks,
This just inverted the body of the loop, suggested by @David Edmondson.
Here is the v2[1]. Do you mean to change it like this?

[1]: 
http://patchwork.ozlabs.org/project/qemu-devel/patch/20210301082132.1107-4-jiangkunkun@huawei.com/

Thanks,
Kunkun Jiang
>> 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 9fc5b2997c..28215aefe4 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -1991,6 +1991,8 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>>       int 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,
>>           int pages_this_iteration = 0;
>>   
>>           /* Check if the page is dirty and send it if it is */
>> -        if (!migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
>> -            pss->page++;
>> -            continue;
>> -        }
>> -
>> -        pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
>> -        if (pages_this_iteration < 0) {
>> -            return pages_this_iteration;
>> -        }
>> +        if (migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
>> +            pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
>> +            if (pages_this_iteration < 0) {
>> +                return pages_this_iteration;
>> +            }
>>   
>> -        pages += pages_this_iteration;
>> -        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 && pages_this_iteration > 0) {
>> -            migration_rate_limit();
>> +            pages += pages_this_iteration;
>> +            /*
>> +             * Allow rate limiting to happen in the middle of huge pages if
>> +             * something is sent in the current iteration.
>> +             */
>> +            if (pagesize_bits > 1 && pages_this_iteration > 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 v3 1/3] migration/ram: Modify the code comment of ram_save_host_page()

[Peter Xu]

On Mon, Mar 08, 2021 at 06:33:56PM +0800, Kunkun Jiang wrote:
> Hi, Peter
> 
> On 2021/3/5 21:59, Peter Xu wrote:
> > On Fri, Mar 05, 2021 at 03:50:33PM +0800, Kunkun Jiang wrote:
> > > The ram_save_host_page() has been modified several times
> > > since its birth. But the comment hasn't been modified as it should
> > > be. It'd better to modify the comment to explain ram_save_host_page()
> > > more clearly.
> > > 
> > > Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> > > ---
> > >   migration/ram.c | 16 +++++++---------
> > >   1 file changed, 7 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 72143da0ac..a168da5cdd 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -1970,15 +1970,13 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
> > >   }
> > >   /**
> > > - * ram_save_host_page: save a whole host page
> > > + * ram_save_host_page: save a whole host page or the rest of a RAMBlock
> > >    *
> > > - * Starting at *offset send pages up to the end of the current host
> > > - * page. It's valid for the initial offset to point into the middle of
> > > - * a host page in which case the remainder of the hostpage is sent.
> > > - * Only dirty target pages are sent. Note that the host page size may
> > > - * be a huge page for this block.

> > > - * The saving stops at the boundary of the used_length of the block
> > > - * if the RAMBlock isn't a multiple of the host page size.

[1]

> > > + * Send dirty pages between pss->page and either the end of that page
> > > + * or the used_length of the RAMBlock, whichever is smaller.
> > > + *
> > > + * Note that if the host page is a huge page, pss->page may be in the
> > > + * middle of that page.
> > It reads more like a shorter version of original comment..  Could you point out
> > what's the major difference?  Since the original comment looks still good to me.
> Sorry for late reply.
> The reason I want to modify the comment is that I think some parts of the
> comment
> don't match the code. (1) The brief description of ram_save_host_page()
> missed a
> situation that ram_save_host_page() will send dirty pages between pass->page
> and
> the used_length of the block, if the RAMBlock isn't a multiple of the host
> page
> size.

It seems it mentioned at [1] above.

> (2) '*offset' should be replaced with pss->page.

This one looks right as you mentioned.

> 
> So taking the chance of optimizing ram_save_host_page(), I modified the
> comment.
> This version comment is suggested by @David Edmondson. Compared with the
> original
> comment, I think it looks concise.

I think changing incorrect comments is nice; it's just that we should still
avoid rewritting comments with similar contents.

> > >    *
> > >    * Returns the number of pages written or negative on error
> > >    *
> > > @@ -2002,7 +2000,7 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
> > >       }
> > >       do {
> > > -        /* Check the pages is dirty and if it is send it */
> > > +        /* Check if the page is dirty and send it if it is */
> > This line fixes some English issues, it seems.  Looks okay, but normally I
> > won't change it unless the wording was too weird, so as to keep the git record
> > or the original lines.  Here the original looks still okay, no strong opinion.
> > 
> > Thanks,
> Yes, the original reads weird to me. Same reason as above, I modified this
> line.
> 
> If you think there is no need to modify the original commit, I do not insist
> on
> changing it.😁

As I mentioned I don't really have a strong preference, so feel free to keep
your own will. :) I would only to suggest avoid rewritting chunk of similar
meanings.

Thanks,

-- 
Peter Xu

Re: [PATCH v3 2/3] migration/ram: Reduce unnecessary rate limiting

[Peter Xu]

On Mon, Mar 08, 2021 at 06:34:58PM +0800, Kunkun Jiang wrote:
> Hi,
> 
> On 2021/3/5 22:22, Peter Xu wrote:
> > Kunkun,
> > 
> > On Fri, Mar 05, 2021 at 03:50:34PM +0800, Kunkun Jiang wrote:
> > > When the host page is a huge page and something is sent in the
> > > current iteration, the migration_rate_limit() should be executed.
> > > If not, this function can be omitted to save time.
> > > 
> > > Rename tmppages to pages_this_iteration to express its meaning
> > > more clearly.
> > > 
> > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > > Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> > > ---
> > >   migration/ram.c | 21 ++++++++++++++-------
> > >   1 file changed, 14 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index a168da5cdd..9fc5b2997c 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -1988,7 +1988,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
> > >   static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
> > >                                 bool last_stage)
> > >   {
> > > -    int tmppages, pages = 0;
> > > +    int pages = 0;
> > >       size_t pagesize_bits =
> > >           qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
> > >       unsigned long start_page = pss->page;
> > > @@ -2000,21 +2000,28 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
> > >       }
> > >       do {
> > > +        int pages_this_iteration = 0;
> > > +
> > >           /* Check if the page is dirty and send it if it is */
> > >           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;
> > > +        pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
> > > +        if (pages_this_iteration < 0) {
> > > +            return pages_this_iteration;
> > >           }
> > > -        pages += tmppages;
> > > +        pages += pages_this_iteration;
> > To me, both names are okay, it's just that the new name doesn't really provide
> > a lot more new information, while it's even longer...
> > 
> > Since you seem to prefer cleaning up tmppages, I'm actually thinking whether
> > it should be called as "pages" at all since ram_save_target_page() majorly only
> > returns either 1 if succeeded or <0 if error.  There's only one very corner
> > case of xbzrle where it can return 0 in save_xbzrle_page():
> > 
> >      if (encoded_len == 0) {
> >          trace_save_xbzrle_page_skipping();
> >          return 0;
> >      }
> > 
> > I think it means the page didn't change at all, then I'm also wondering maybe
> > it can also return 1 showing one page migrated (though actually skipped!) which
> > should still be fine for the callers, e.g., ram_find_and_save_block() who will
> > finally check this "pages" value.
> > 
> > So I think _maybe_ that's a nicer cleanup to change that "return 0" to "return
> > 1", then another patch to make the return value to be (1) return 0 if page
> > saved, or (2) return <0 if error.  Then here in ram_save_host_page() tmppages
> > can be renamed to "ret" or "succeed".
> Thanks for your advice.
> change "return 0" to "return 1" would have a slight effect on
> 'rs->target_page_count += pages'
> in ram_save_iterate(). This may lead to consider more complex situations.
> What do you think of
> this?

I don't think we should change the meaning of ram_save_host_page()'s return
value, but only ram_save_target_page(); ram_save_host_page() could return >1
for huge pages.  Thanks,

-- 
Peter Xu

Re: [PATCH v3 3/3] migration/ram: Optimize ram_save_host_page()

[Peter Xu]

On Mon, Mar 08, 2021 at 09:58:02PM +0800, Kunkun Jiang wrote:
> Hi,
> 
> On 2021/3/5 22:30, Peter Xu wrote:
> > On Fri, Mar 05, 2021 at 03:50:35PM +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.
> > > 
> > > This will improve performance to use migration_bitmap_find_dirty().
> > Is there any measurement done?
> I tested it on Kunpeng 920.  VM params: 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

Looks like an idle VM, but still this is a great improvement.  Would you mind
add this into the commit message too?

> > This looks like an optimization, but to me it seems to have changed a lot
> > context that it doesn't need to... Do you think it'll also work to just look up
> > dirty again and update pss->page properly if migration_bitmap_clear_dirty()
> > returned zero?
> > 
> > Thanks,
> This just inverted the body of the loop, suggested by @David Edmondson.
> Here is the v2[1]. Do you mean to change it like this?
> 
> [1]: http://patchwork.ozlabs.org/project/qemu-devel/patch/20210301082132.1107-4-jiangkunkun@huawei.com/

I see, then it's okay - But indeed I still prefer your previous version. :)

Thanks,

-- 
Peter Xu

Re: [PATCH v3 1/3] migration/ram: Modify the code comment of ram_save_host_page()

[Kunkun Jiang]

Hi,

On 2021/3/9 5:03, Peter Xu wrote:
> On Mon, Mar 08, 2021 at 06:33:56PM +0800, Kunkun Jiang wrote:
>> Hi, Peter
>>
>> On 2021/3/5 21:59, Peter Xu wrote:
>>> On Fri, Mar 05, 2021 at 03:50:33PM +0800, Kunkun Jiang wrote:
>>>> The ram_save_host_page() has been modified several times
>>>> since its birth. But the comment hasn't been modified as it should
>>>> be. It'd better to modify the comment to explain ram_save_host_page()
>>>> more clearly.
>>>>
>>>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>>>> ---
>>>>    migration/ram.c | 16 +++++++---------
>>>>    1 file changed, 7 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>> index 72143da0ac..a168da5cdd 100644
>>>> --- a/migration/ram.c
>>>> +++ b/migration/ram.c
>>>> @@ -1970,15 +1970,13 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>>>>    }
>>>>    /**
>>>> - * ram_save_host_page: save a whole host page
>>>> + * ram_save_host_page: save a whole host page or the rest of a RAMBlock
>>>>     *
>>>> - * Starting at *offset send pages up to the end of the current host
>>>> - * page. It's valid for the initial offset to point into the middle of
>>>> - * a host page in which case the remainder of the hostpage is sent.
>>>> - * Only dirty target pages are sent. Note that the host page size may
>>>> - * be a huge page for this block.
>>>> - * The saving stops at the boundary of the used_length of the block
>>>> - * if the RAMBlock isn't a multiple of the host page size.
> [1]
>
>>>> + * Send dirty pages between pss->page and either the end of that page
>>>> + * or the used_length of the RAMBlock, whichever is smaller.
>>>> + *
>>>> + * Note that if the host page is a huge page, pss->page may be in the
>>>> + * middle of that page.
>>> It reads more like a shorter version of original comment..  Could you point out
>>> what's the major difference?  Since the original comment looks still good to me.
>> Sorry for late reply.
>> The reason I want to modify the comment is that I think some parts of the
>> comment
>> don't match the code. (1) The brief description of ram_save_host_page()
>> missed a
>> situation that ram_save_host_page() will send dirty pages between pass->page
>> and
>> the used_length of the block, if the RAMBlock isn't a multiple of the host
>> page
>> size.
> It seems it mentioned at [1] above.
>
>> (2) '*offset' should be replaced with pss->page.
> This one looks right as you mentioned.
>
>> So taking the chance of optimizing ram_save_host_page(), I modified the
>> comment.
>> This version comment is suggested by @David Edmondson. Compared with the
>> original
>> comment, I think it looks concise.
> I think changing incorrect comments is nice; it's just that we should still
> avoid rewritting comments with similar contents.
>
>>>>     *
>>>>     * Returns the number of pages written or negative on error
>>>>     *
>>>> @@ -2002,7 +2000,7 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>>>>        }
>>>>        do {
>>>> -        /* Check the pages is dirty and if it is send it */
>>>> +        /* Check if the page is dirty and send it if it is */
>>> This line fixes some English issues, it seems.  Looks okay, but normally I
>>> won't change it unless the wording was too weird, so as to keep the git record
>>> or the original lines.  Here the original looks still okay, no strong opinion.
>>>
>>> Thanks,
>> Yes, the original reads weird to me. Same reason as above, I modified this
>> line.
>>
>> If you think there is no need to modify the original commit, I do not insist
>> on
>> changing it.😁
> As I mentioned I don't really have a strong preference, so feel free to keep
> your own will. :) I would only to suggest avoid rewritting chunk of similar
> meanings.
>
> Thanks,
>
Thank you for your advice, I will pay more attention to it in the future.

Best regards,

Kunkun Jiang

Re: [PATCH v3 3/3] migration/ram: Optimize ram_save_host_page()

[Kunkun Jiang]

Hi,

On 2021/3/9 5:36, Peter Xu wrote:
> On Mon, Mar 08, 2021 at 09:58:02PM +0800, Kunkun Jiang wrote:
>> Hi,
>>
>> On 2021/3/5 22:30, Peter Xu wrote:
>>> On Fri, Mar 05, 2021 at 03:50:35PM +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.
>>>>
>>>> This will improve performance to use migration_bitmap_find_dirty().
>>> Is there any measurement done?
>> I tested it on Kunpeng 920.  VM params: 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
> Looks like an idle VM, but still this is a great improvement.  Would you mind
> add this into the commit message too?
Ok, I will add it in the next version.😉
>>> This looks like an optimization, but to me it seems to have changed a lot
>>> context that it doesn't need to... Do you think it'll also work to just look up
>>> dirty again and update pss->page properly if migration_bitmap_clear_dirty()
>>> returned zero?
>>>
>>> Thanks,
>> This just inverted the body of the loop, suggested by @David Edmondson.
>> Here is the v2[1]. Do you mean to change it like this?
>>
>> [1]: http://patchwork.ozlabs.org/project/qemu-devel/patch/20210301082132.1107-4-jiangkunkun@huawei.com/
> I see, then it's okay - But indeed I still prefer your previous version. :)
>
> Thanks,
>
Both versions are fine to me. This version may make the final code 
slightly cleaner, I think.

Thanks,

Kunkun Jiang

Re: [PATCH v3 2/3] migration/ram: Reduce unnecessary rate limiting

[Kunkun Jiang]

Hi,

On 2021/3/9 5:12, Peter Xu wrote:
> On Mon, Mar 08, 2021 at 06:34:58PM +0800, Kunkun Jiang wrote:
>> Hi,
>>
>> On 2021/3/5 22:22, Peter Xu wrote:
>>> Kunkun,
>>>
>>> On Fri, Mar 05, 2021 at 03:50:34PM +0800, Kunkun Jiang wrote:
>>>> When the host page is a huge page and something is sent in the
>>>> current iteration, the migration_rate_limit() should be executed.
>>>> If not, this function can be omitted to save time.
>>>>
>>>> Rename tmppages to pages_this_iteration to express its meaning
>>>> more clearly.
>>>>
>>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>>>> ---
>>>>    migration/ram.c | 21 ++++++++++++++-------
>>>>    1 file changed, 14 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>> index a168da5cdd..9fc5b2997c 100644
>>>> --- a/migration/ram.c
>>>> +++ b/migration/ram.c
>>>> @@ -1988,7 +1988,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>>>>    static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>>>>                                  bool last_stage)
>>>>    {
>>>> -    int tmppages, pages = 0;
>>>> +    int pages = 0;
>>>>        size_t pagesize_bits =
>>>>            qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
>>>>        unsigned long start_page = pss->page;
>>>> @@ -2000,21 +2000,28 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>>>>        }
>>>>        do {
>>>> +        int pages_this_iteration = 0;
>>>> +
>>>>            /* Check if the page is dirty and send it if it is */
>>>>            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;
>>>> +        pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
>>>> +        if (pages_this_iteration < 0) {
>>>> +            return pages_this_iteration;
>>>>            }
>>>> -        pages += tmppages;
>>>> +        pages += pages_this_iteration;
>>> To me, both names are okay, it's just that the new name doesn't really provide
>>> a lot more new information, while it's even longer...
>>>
>>> Since you seem to prefer cleaning up tmppages, I'm actually thinking whether
>>> it should be called as "pages" at all since ram_save_target_page() majorly only
>>> returns either 1 if succeeded or <0 if error.  There's only one very corner
>>> case of xbzrle where it can return 0 in save_xbzrle_page():
>>>
>>>       if (encoded_len == 0) {
>>>           trace_save_xbzrle_page_skipping();
>>>           return 0;
>>>       }
>>>
>>> I think it means the page didn't change at all, then I'm also wondering maybe
>>> it can also return 1 showing one page migrated (though actually skipped!) which
>>> should still be fine for the callers, e.g., ram_find_and_save_block() who will
>>> finally check this "pages" value.
>>>
>>> So I think _maybe_ that's a nicer cleanup to change that "return 0" to "return
>>> 1", then another patch to make the return value to be (1) return 0 if page
>>> saved, or (2) return <0 if error.  Then here in ram_save_host_page() tmppages
>>> can be renamed to "ret" or "succeed".
>> Thanks for your advice.
>> change "return 0" to "return 1" would have a slight effect on
>> 'rs->target_page_count += pages'
>> in ram_save_iterate(). This may lead to consider more complex situations.
>> What do you think of
>> this?
> I don't think we should change the meaning of ram_save_host_page()'s return
> value, but only ram_save_target_page(); ram_save_host_page() could return >1
> for huge pages.  Thanks,
>
I am not sure I have got your point. If I change "return 0" to "return 
1" (actually skipped),
the "pages" in the ram_save_host_page() will add 1(original add 0). Then 
it will end the
loop in the ram_find_and_save_block. And then in the ram_save_iterate(), 
the modify may
have a effect on "rs->target_page_count += page".
I haven't done enough research on this part of code. Do you think this 
change will have
a big impact?

Thanks,

Kunkun Jiang

Re: [PATCH v3 2/3] migration/ram: Reduce unnecessary rate limiting

[Peter Xu]

On Tue, Mar 09, 2021 at 10:33:04PM +0800, Kunkun Jiang wrote:
> Hi,
> 
> On 2021/3/9 5:12, Peter Xu wrote:
> > On Mon, Mar 08, 2021 at 06:34:58PM +0800, Kunkun Jiang wrote:
> > > Hi,
> > > 
> > > On 2021/3/5 22:22, Peter Xu wrote:
> > > > Kunkun,
> > > > 
> > > > On Fri, Mar 05, 2021 at 03:50:34PM +0800, Kunkun Jiang wrote:
> > > > > When the host page is a huge page and something is sent in the
> > > > > current iteration, the migration_rate_limit() should be executed.
> > > > > If not, this function can be omitted to save time.
> > > > > 
> > > > > Rename tmppages to pages_this_iteration to express its meaning
> > > > > more clearly.
> > > > > 
> > > > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > > > > Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> > > > > ---
> > > > >    migration/ram.c | 21 ++++++++++++++-------
> > > > >    1 file changed, 14 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/migration/ram.c b/migration/ram.c
> > > > > index a168da5cdd..9fc5b2997c 100644
> > > > > --- a/migration/ram.c
> > > > > +++ b/migration/ram.c
> > > > > @@ -1988,7 +1988,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
> > > > >    static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
> > > > >                                  bool last_stage)
> > > > >    {
> > > > > -    int tmppages, pages = 0;
> > > > > +    int pages = 0;
> > > > >        size_t pagesize_bits =
> > > > >            qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
> > > > >        unsigned long start_page = pss->page;
> > > > > @@ -2000,21 +2000,28 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
> > > > >        }
> > > > >        do {
> > > > > +        int pages_this_iteration = 0;
> > > > > +
> > > > >            /* Check if the page is dirty and send it if it is */
> > > > >            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;
> > > > > +        pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
> > > > > +        if (pages_this_iteration < 0) {
> > > > > +            return pages_this_iteration;
> > > > >            }
> > > > > -        pages += tmppages;
> > > > > +        pages += pages_this_iteration;
> > > > To me, both names are okay, it's just that the new name doesn't really provide
> > > > a lot more new information, while it's even longer...
> > > > 
> > > > Since you seem to prefer cleaning up tmppages, I'm actually thinking whether
> > > > it should be called as "pages" at all since ram_save_target_page() majorly only
> > > > returns either 1 if succeeded or <0 if error.  There's only one very corner
> > > > case of xbzrle where it can return 0 in save_xbzrle_page():
> > > > 
> > > >       if (encoded_len == 0) {
> > > >           trace_save_xbzrle_page_skipping();
> > > >           return 0;
> > > >       }
> > > > 
> > > > I think it means the page didn't change at all, then I'm also wondering maybe
> > > > it can also return 1 showing one page migrated (though actually skipped!) which
> > > > should still be fine for the callers, e.g., ram_find_and_save_block() who will
> > > > finally check this "pages" value.
> > > > 
> > > > So I think _maybe_ that's a nicer cleanup to change that "return 0" to "return
> > > > 1", then another patch to make the return value to be (1) return 0 if page
> > > > saved, or (2) return <0 if error.  Then here in ram_save_host_page() tmppages
> > > > can be renamed to "ret" or "succeed".
> > > Thanks for your advice.
> > > change "return 0" to "return 1" would have a slight effect on
> > > 'rs->target_page_count += pages'
> > > in ram_save_iterate(). This may lead to consider more complex situations.
> > > What do you think of
> > > this?
> > I don't think we should change the meaning of ram_save_host_page()'s return
> > value, but only ram_save_target_page(); ram_save_host_page() could return >1
> > for huge pages.  Thanks,
> > 
> I am not sure I have got your point. If I change "return 0" to "return 1"
> (actually skipped),
> the "pages" in the ram_save_host_page() will add 1(original add 0). Then it
> will end the
> loop in the ram_find_and_save_block.

Frankly I even think it's a bug - when return 0 it could mean the xbzrle page
is the same as before even if dirty bit set (the page just got written with the
same data, that's why dirty bit set but xbzrle calculated with no diff).
However it shouldn't mean "done==1" which is a sign of "migration complete"
imho..

> And then in the ram_save_iterate(), the
> modify may
> have a effect on "rs->target_page_count += page".
> I haven't done enough research on this part of code. Do you think this
> change will have
> a big impact?

Maybe, but I don't expect it to change anything real.  If to change it we'd
definitely better smoke xbzrle a bit.  It's a pure idea I got in mind to
cleanup the code, but feel free to ignore it too.

For your current series, I think the last patch is the most appealing.  So
maybe we can focus on that first.

Thanks,

-- 
Peter Xu

Re: [PATCH v3 2/3] migration/ram: Reduce unnecessary rate limiting

[Kunkun Jiang]

Hi,

On 2021/3/10 0:15, Peter Xu wrote:
> On Tue, Mar 09, 2021 at 10:33:04PM +0800, Kunkun Jiang wrote:
>> Hi,
>>
>> On 2021/3/9 5:12, Peter Xu wrote:
>>> On Mon, Mar 08, 2021 at 06:34:58PM +0800, Kunkun Jiang wrote:
>>>> Hi,
>>>>
>>>> On 2021/3/5 22:22, Peter Xu wrote:
>>>>> Kunkun,
>>>>>
>>>>> On Fri, Mar 05, 2021 at 03:50:34PM +0800, Kunkun Jiang wrote:
>>>>>> When the host page is a huge page and something is sent in the
>>>>>> current iteration, the migration_rate_limit() should be executed.
>>>>>> If not, this function can be omitted to save time.
>>>>>>
>>>>>> Rename tmppages to pages_this_iteration to express its meaning
>>>>>> more clearly.
>>>>>>
>>>>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>>>>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>>>>>> ---
>>>>>>     migration/ram.c | 21 ++++++++++++++-------
>>>>>>     1 file changed, 14 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/migration/ram.c b/migration/ram.c
>>>>>> index a168da5cdd..9fc5b2997c 100644
>>>>>> --- a/migration/ram.c
>>>>>> +++ b/migration/ram.c
>>>>>> @@ -1988,7 +1988,7 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>>>>>>     static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>>>>>>                                   bool last_stage)
>>>>>>     {
>>>>>> -    int tmppages, pages = 0;
>>>>>> +    int pages = 0;
>>>>>>         size_t pagesize_bits =
>>>>>>             qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
>>>>>>         unsigned long start_page = pss->page;
>>>>>> @@ -2000,21 +2000,28 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>>>>>>         }
>>>>>>         do {
>>>>>> +        int pages_this_iteration = 0;
>>>>>> +
>>>>>>             /* Check if the page is dirty and send it if it is */
>>>>>>             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;
>>>>>> +        pages_this_iteration = ram_save_target_page(rs, pss, last_stage);
>>>>>> +        if (pages_this_iteration < 0) {
>>>>>> +            return pages_this_iteration;
>>>>>>             }
>>>>>> -        pages += tmppages;
>>>>>> +        pages += pages_this_iteration;
>>>>> To me, both names are okay, it's just that the new name doesn't really provide
>>>>> a lot more new information, while it's even longer...
>>>>>
>>>>> Since you seem to prefer cleaning up tmppages, I'm actually thinking whether
>>>>> it should be called as "pages" at all since ram_save_target_page() majorly only
>>>>> returns either 1 if succeeded or <0 if error.  There's only one very corner
>>>>> case of xbzrle where it can return 0 in save_xbzrle_page():
>>>>>
>>>>>        if (encoded_len == 0) {
>>>>>            trace_save_xbzrle_page_skipping();
>>>>>            return 0;
>>>>>        }
>>>>>
>>>>> I think it means the page didn't change at all, then I'm also wondering maybe
>>>>> it can also return 1 showing one page migrated (though actually skipped!) which
>>>>> should still be fine for the callers, e.g., ram_find_and_save_block() who will
>>>>> finally check this "pages" value.
>>>>>
>>>>> So I think _maybe_ that's a nicer cleanup to change that "return 0" to "return
>>>>> 1", then another patch to make the return value to be (1) return 0 if page
>>>>> saved, or (2) return <0 if error.  Then here in ram_save_host_page() tmppages
>>>>> can be renamed to "ret" or "succeed".
>>>> Thanks for your advice.
>>>> change "return 0" to "return 1" would have a slight effect on
>>>> 'rs->target_page_count += pages'
>>>> in ram_save_iterate(). This may lead to consider more complex situations.
>>>> What do you think of
>>>> this?
>>> I don't think we should change the meaning of ram_save_host_page()'s return
>>> value, but only ram_save_target_page(); ram_save_host_page() could return >1
>>> for huge pages.  Thanks,
>>>
>> I am not sure I have got your point. If I change "return 0" to "return 1"
>> (actually skipped),
>> the "pages" in the ram_save_host_page() will add 1(original add 0). Then it
>> will end the
>> loop in the ram_find_and_save_block.
> Frankly I even think it's a bug - when return 0 it could mean the xbzrle page
> is the same as before even if dirty bit set (the page just got written with the
> same data, that's why dirty bit set but xbzrle calculated with no diff).
> However it shouldn't mean "done==1" which is a sign of "migration complete"
> imho..
Thanks for your explanation, I get it.
>> And then in the ram_save_iterate(), the
>> modify may
>> have a effect on "rs->target_page_count += page".
>> I haven't done enough research on this part of code. Do you think this
>> change will have
>> a big impact?
> Maybe, but I don't expect it to change anything real.  If to change it we'd
> definitely better smoke xbzrle a bit.  It's a pure idea I got in mind to
> cleanup the code, but feel free to ignore it too.
>
> For your current series, I think the last patch is the most appealing.  So
> maybe we can focus on that first.
>
> Thanks,
>
You are right. The change here may be not worth it.

Thanks,

Kunkun Jiang