Re: [Qemu-devel] [PATCH 4/8] migration: introduce control_save_page()

[Peter Xu]

On Thu, Mar 15, 2018 at 11:37:59AM +, Dr. David Alan Gilbert wrote:
> * guangrong.x...@gmail.com (guangrong.x...@gmail.com) wrote:
> > From: Xiao Guangrong 
> > 
> > Abstract the common function control_save_page() to cleanup the code,
> > no logic is changed
> > 
> > Signed-off-by: Xiao Guangrong 
> 
> Reviewed-by: Dr. David Alan Gilbert 
> 
> It would be good to find a better name for control_save_page, but I
> can't think of one!).

Yeah.  I would prefer it's at least still prefixed with ram_*, however
I don't really hope we spend too much time on namings (always :).

Maybe we can just squash the changes into current
ram_control_save_page() directly.  But that's optional, current patch
is good to me already, so:

Reviewed-by: Peter Xu 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 4/8] migration: introduce control_save_page()

[Xiao Guangrong]

On 03/15/2018 07:37 PM, Dr. David Alan Gilbert wrote:

* guangrong.x...@gmail.com (guangrong.x...@gmail.com) wrote:

From: Xiao Guangrong 

Abstract the common function control_save_page() to cleanup the code,
no logic is changed

Signed-off-by: Xiao Guangrong 


Reviewed-by: Dr. David Alan Gilbert 



Thank you, Dave!

  
+/*

+ * @pages: the number of pages written by the control path,
+ *< 0 - error
+ *> 0 - number of pages written


What about 0 !



The control patch does not support 0 (means duplication). :)

Based on the implementation of qemu_rdma_save_page(), if any data
is properly posted out, @bytes_sent is set to 1, otherwise a error
is detected...

Re: [Qemu-devel] [PATCH 4/8] migration: introduce control_save_page()

[Dr. David Alan Gilbert]

* guangrong.x...@gmail.com (guangrong.x...@gmail.com) wrote:
> From: Xiao Guangrong 
> 
> Abstract the common function control_save_page() to cleanup the code,
> no logic is changed
> 
> Signed-off-by: Xiao Guangrong 

Reviewed-by: Dr. David Alan Gilbert 

It would be good to find a better name for control_save_page, but I
can't think of one!).
> ---
>  migration/ram.c | 174 
> +---
>  1 file changed, 89 insertions(+), 85 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index c47185d38c..e7b8b14c3c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -957,6 +957,44 @@ static void ram_release_pages(const char *rbname, 
> uint64_t offset, int pages)
>  ram_discard_range(rbname, offset, pages << TARGET_PAGE_BITS);
>  }
>  
> +/*
> + * @pages: the number of pages written by the control path,
> + *< 0 - error
> + *> 0 - number of pages written

What about 0 !

> + * Return true if the pages has been saved, otherwise false is returned.
> + */
> +static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t 
> offset,
> +  int *pages)
> +{
> +uint64_t bytes_xmit = 0;
> +int ret;
> +
> +*pages = -1;
> +ret = ram_control_save_page(rs->f, block->offset, offset, 
> TARGET_PAGE_SIZE,
> +_xmit);
> +if (ret == RAM_SAVE_CONTROL_NOT_SUPP) {
> +return false;
> +}
> +
> +if (bytes_xmit) {
> +ram_counters.transferred += bytes_xmit;
> +*pages = 1;
> +}
> +
> +if (ret == RAM_SAVE_CONTROL_DELAYED) {
> +return true;
> +}
> +
> +if (bytes_xmit > 0) {
> +ram_counters.normal++;
> +} else if (bytes_xmit == 0) {
> +ram_counters.duplicate++;
> +}
> +
> +return true;
> +}
> +
>  /**
>   * ram_save_page: send the given page to the stream
>   *
> @@ -973,56 +1011,36 @@ static void ram_release_pages(const char *rbname, 
> uint64_t offset, int pages)
>  static int ram_save_page(RAMState *rs, PageSearchStatus *pss, bool 
> last_stage)
>  {
>  int pages = -1;
> -uint64_t bytes_xmit;
> -ram_addr_t current_addr;
>  uint8_t *p;
> -int ret;
>  bool send_async = true;
>  RAMBlock *block = pss->block;
>  ram_addr_t offset = pss->page << TARGET_PAGE_BITS;
> +ram_addr_t current_addr = block->offset + offset;
>  
>  p = block->host + offset;
>  trace_ram_save_page(block->idstr, (uint64_t)offset, p);
>  
> -/* In doubt sent page as normal */
> -bytes_xmit = 0;
> -ret = ram_control_save_page(rs->f, block->offset,
> -   offset, TARGET_PAGE_SIZE, _xmit);
> -if (bytes_xmit) {
> -ram_counters.transferred += bytes_xmit;
> -pages = 1;
> +if (control_save_page(rs, block, offset, )) {
> +return pages;
>  }
>  
>  XBZRLE_cache_lock();
> -
> -current_addr = block->offset + offset;
> -
> -if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
> -if (ret != RAM_SAVE_CONTROL_DELAYED) {
> -if (bytes_xmit > 0) {
> -ram_counters.normal++;
> -} else if (bytes_xmit == 0) {
> -ram_counters.duplicate++;
> -}
> -}
> -} else {
> -pages = save_zero_page(rs, block, offset);
> -if (pages > 0) {
> -/* Must let xbzrle know, otherwise a previous (now 0'd) cached
> - * page would be stale
> +pages = save_zero_page(rs, block, offset);
> +if (pages > 0) {
> +/* Must let xbzrle know, otherwise a previous (now 0'd) cached
> + * page would be stale
> + */
> +xbzrle_cache_zero_page(rs, current_addr);
> +ram_release_pages(block->idstr, offset, pages);
> +} else if (!rs->ram_bulk_stage &&
> +   !migration_in_postcopy() && migrate_use_xbzrle()) {
> +pages = save_xbzrle_page(rs, , current_addr, block,
> + offset, last_stage);
> +if (!last_stage) {
> +/* Can't send this cached data async, since the cache page
> + * might get updated before it gets to the wire
>   */
> -xbzrle_cache_zero_page(rs, current_addr);
> -ram_release_pages(block->idstr, offset, pages);
> -} else if (!rs->ram_bulk_stage &&
> -   !migration_in_postcopy() && migrate_use_xbzrle()) {
> -pages = save_xbzrle_page(rs, , current_addr, block,
> - offset, last_stage);
> -if (!last_stage) {
> -/* Can't send this cached data async, since the cache page
> - * might get updated before it gets to the wire
> - */
> -send_async = false;
> -}
> +send_async = false;
>  }
>  }
>  
> @@ -1152,63 +1170,49 @@ 

[Qemu-devel] [PATCH 4/8] migration: introduce control_save_page()

[guangrong . xiao]

From: Xiao Guangrong 

Abstract the common function control_save_page() to cleanup the code,
no logic is changed

Signed-off-by: Xiao Guangrong 
---
 migration/ram.c | 174 +---
 1 file changed, 89 insertions(+), 85 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index c47185d38c..e7b8b14c3c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -957,6 +957,44 @@ static void ram_release_pages(const char *rbname, uint64_t 
offset, int pages)
 ram_discard_range(rbname, offset, pages << TARGET_PAGE_BITS);
 }
 
+/*
+ * @pages: the number of pages written by the control path,
+ *< 0 - error
+ *> 0 - number of pages written
+ *
+ * Return true if the pages has been saved, otherwise false is returned.
+ */
+static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
+  int *pages)
+{
+uint64_t bytes_xmit = 0;
+int ret;
+
+*pages = -1;
+ret = ram_control_save_page(rs->f, block->offset, offset, TARGET_PAGE_SIZE,
+_xmit);
+if (ret == RAM_SAVE_CONTROL_NOT_SUPP) {
+return false;
+}
+
+if (bytes_xmit) {
+ram_counters.transferred += bytes_xmit;
+*pages = 1;
+}
+
+if (ret == RAM_SAVE_CONTROL_DELAYED) {
+return true;
+}
+
+if (bytes_xmit > 0) {
+ram_counters.normal++;
+} else if (bytes_xmit == 0) {
+ram_counters.duplicate++;
+}
+
+return true;
+}
+
 /**
  * ram_save_page: send the given page to the stream
  *
@@ -973,56 +1011,36 @@ static void ram_release_pages(const char *rbname, 
uint64_t offset, int pages)
 static int ram_save_page(RAMState *rs, PageSearchStatus *pss, bool last_stage)
 {
 int pages = -1;
-uint64_t bytes_xmit;
-ram_addr_t current_addr;
 uint8_t *p;
-int ret;
 bool send_async = true;
 RAMBlock *block = pss->block;
 ram_addr_t offset = pss->page << TARGET_PAGE_BITS;
+ram_addr_t current_addr = block->offset + offset;
 
 p = block->host + offset;
 trace_ram_save_page(block->idstr, (uint64_t)offset, p);
 
-/* In doubt sent page as normal */
-bytes_xmit = 0;
-ret = ram_control_save_page(rs->f, block->offset,
-   offset, TARGET_PAGE_SIZE, _xmit);
-if (bytes_xmit) {
-ram_counters.transferred += bytes_xmit;
-pages = 1;
+if (control_save_page(rs, block, offset, )) {
+return pages;
 }
 
 XBZRLE_cache_lock();
-
-current_addr = block->offset + offset;
-
-if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
-if (ret != RAM_SAVE_CONTROL_DELAYED) {
-if (bytes_xmit > 0) {
-ram_counters.normal++;
-} else if (bytes_xmit == 0) {
-ram_counters.duplicate++;
-}
-}
-} else {
-pages = save_zero_page(rs, block, offset);
-if (pages > 0) {
-/* Must let xbzrle know, otherwise a previous (now 0'd) cached
- * page would be stale
+pages = save_zero_page(rs, block, offset);
+if (pages > 0) {
+/* Must let xbzrle know, otherwise a previous (now 0'd) cached
+ * page would be stale
+ */
+xbzrle_cache_zero_page(rs, current_addr);
+ram_release_pages(block->idstr, offset, pages);
+} else if (!rs->ram_bulk_stage &&
+   !migration_in_postcopy() && migrate_use_xbzrle()) {
+pages = save_xbzrle_page(rs, , current_addr, block,
+ offset, last_stage);
+if (!last_stage) {
+/* Can't send this cached data async, since the cache page
+ * might get updated before it gets to the wire
  */
-xbzrle_cache_zero_page(rs, current_addr);
-ram_release_pages(block->idstr, offset, pages);
-} else if (!rs->ram_bulk_stage &&
-   !migration_in_postcopy() && migrate_use_xbzrle()) {
-pages = save_xbzrle_page(rs, , current_addr, block,
- offset, last_stage);
-if (!last_stage) {
-/* Can't send this cached data async, since the cache page
- * might get updated before it gets to the wire
- */
-send_async = false;
-}
+send_async = false;
 }
 }
 
@@ -1152,63 +1170,49 @@ static int ram_save_compressed_page(RAMState *rs, 
PageSearchStatus *pss,
 bool last_stage)
 {
 int pages = -1;
-uint64_t bytes_xmit = 0;
 uint8_t *p;
-int ret;
 RAMBlock *block = pss->block;
 ram_addr_t offset = pss->page << TARGET_PAGE_BITS;
 
 p = block->host + offset;
 
-ret = ram_control_save_page(rs->f, block->offset,
-offset, TARGET_PAGE_SIZE, _xmit);
-if (bytes_xmit) {
-