[PATCH v2 00/20] Next round of migration atomic counters

[PATCH v2 00/20] Next round of migration atomic counters

[Juan Quintela]

Hi

On this v2:

- dropped qemu_fflush() assert for read only files (make Richard
  happy)

- Update documentation for qemu_file_transferred (make Fabiano happy)

- migration/rdma: Remove qemu_fopen_rdma() and make it look like
  everything else

- Simplify a couple of qemu-file functions, and unexport the ones that
  are not used outside of qemu-file.c

- Added Reviewed-by comments.

Please review.

Thanks, Juan.

[v1]
On this series:

- Make sure that qemu_file_transferred() make sense and its used
  coherently

- Use stat64 for qemu_file_transferred(), so we can call the function
  from any thread.

- Don't account for the same transfer twice (i.e. it is multifd_bytes,
  rdma_bytes or qemu_file_bytes) qemu_file_transferred() just sums all
  of them.

- Use this new counter for rate_limit()

- Remove old trasferred stat64 (now we use the real thing)

- Simplify qemu_file_get_error(): see where next cleanups are coming

- As an example, qemu_fflush() now return errors.

Please review.

Later, Juan.

Based-on: Message-Id: <20230530115429.1998-1-quintela@redhat.com>
Subject: [PULL 00/21] Migration 20230530 patches

Juan Quintela (20):
  qemu-file: Rename qemu_file_transferred_ fast -> noflush
  migration: Change qemu_file_transferred to noflush
  migration: Use qemu_file_transferred_noflush() for block migration.
  qemu-file: We only call qemu_file_transferred_* on the sending side
  qemu_file: Use a stat64 for qemu_file_transferred
  qemu_file: total_transferred is not used anymore
  migration: Use the number of transferred bytes directly
  qemu_file: Remove unused qemu_file_transferred()
  qemu-file: Remove _noflush from qemu_file_transferred_noflush()
  migration: migration_transferred_bytes() don't need the QEMUFile
  migration: migration_rate_limit_reset() don't need the QEMUFile
  qemu-file: Simplify qemu_file_get_error()
  migration: Use migration_transferred_bytes()
  migration: Remove transferred atomic counter
  qemu-file: Make qemu_fflush() return errors
  migration/rdma: Split qemu_fopen_rdma() into input/output functions
  qemu-file: Remove unused qemu_file_mode_is_not_valid()
  qemu_file: Make qemu_file_is_writable() static
  qemu-file: Simplify qemu_file_shutdown()
  qemu-file: Make qemu_file_get_error_obj() static

 migration/migration-stats.h | 16 ++++-----
 migration/qemu-file.h       | 31 +++---------------
 migration/colo.c            | 11 ++-----
 migration/migration-stats.c | 10 +++---
 migration/migration.c       | 17 ++++------
 migration/multifd.c         |  3 --
 migration/qemu-file.c       | 65 ++++++++++---------------------------
 migration/ram.c             | 29 ++++++-----------
 migration/rdma.c            | 39 ++++++++++------------
 migration/savevm.c          |  7 ++--
 migration/vmstate.c         |  4 +--
 11 files changed, 73 insertions(+), 159 deletions(-)

-- 
2.40.1

[PATCH v2 01/20] qemu-file: Rename qemu_file_transferred_ fast -> noflush

[Juan Quintela]

Fast don't say much.  Noflush indicates more clearly that it is like
qemu_file_transferred but without the flush.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.h | 11 +++++------
 migration/qemu-file.c |  2 +-
 migration/savevm.c    |  4 ++--
 migration/vmstate.c   |  4 ++--
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 83b8fb10de..21c3f34b84 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -52,16 +52,15 @@ int qemu_fclose(QEMUFile *f);
 uint64_t qemu_file_transferred(QEMUFile *f);
 
 /*
- * qemu_file_transferred_fast:
+ * qemu_file_transferred_noflush:
  *
- * As qemu_file_transferred except for writable
- * files, where no flush is performed and the reported
- * amount will include the size of any queued buffers,
- * on top of the amount actually transferred.
+ * As qemu_file_transferred except for writable files, where no flush
+ * is performed and the reported amount will include the size of any
+ * queued buffers, on top of the amount actually transferred.
  *
  * Returns: the total bytes transferred and queued
  */
-uint64_t qemu_file_transferred_fast(QEMUFile *f);
+uint64_t qemu_file_transferred_noflush(QEMUFile *f);
 
 /*
  * put_buffer without copying the buffer.
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index c94b667726..7a5c1a5e32 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -627,7 +627,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
     return result;
 }
 
-uint64_t qemu_file_transferred_fast(QEMUFile *f)
+uint64_t qemu_file_transferred_noflush(QEMUFile *f)
 {
     uint64_t ret = f->total_transferred;
     int i;
diff --git a/migration/savevm.c b/migration/savevm.c
index bc284087f9..f26b455764 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -927,9 +927,9 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
 static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
                                    JSONWriter *vmdesc)
 {
-    uint64_t old_offset = qemu_file_transferred_fast(f);
+    uint64_t old_offset = qemu_file_transferred_noflush(f);
     se->ops->save_state(f, se->opaque);
-    uint64_t size = qemu_file_transferred_fast(f) - old_offset;
+    uint64_t size = qemu_file_transferred_noflush(f) - old_offset;
 
     if (vmdesc) {
         json_writer_int64(vmdesc, "size", size);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index af01d54b6f..31842c3afb 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -361,7 +361,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 void *curr_elem = first_elem + size * i;
 
                 vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
-                old_offset = qemu_file_transferred_fast(f);
+                old_offset = qemu_file_transferred_noflush(f);
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
                     assert(curr_elem);
                     curr_elem = *(void **)curr_elem;
@@ -391,7 +391,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                     return ret;
                 }
 
-                written_bytes = qemu_file_transferred_fast(f) - old_offset;
+                written_bytes = qemu_file_transferred_noflush(f) - old_offset;
                 vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i);
 
                 /* Compressed arrays only care about the first element */
-- 
2.40.1

[PATCH v2 02/20] migration: Change qemu_file_transferred to noflush

[Juan Quintela]

We do a qemu_fclose() just after that, that also does a qemu_fflush(),
so remove one qemu_fflush().

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/savevm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index f26b455764..b2199d1039 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2952,7 +2952,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
         goto the_end;
     }
     ret = qemu_savevm_state(f, errp);
-    vm_state_size = qemu_file_transferred(f);
+    vm_state_size = qemu_file_transferred_noflush(f);
     ret2 = qemu_fclose(f);
     if (ret < 0) {
         goto the_end;
-- 
2.40.1

[PATCH v2 03/20] migration: Use qemu_file_transferred_noflush() for block migration.

[Juan Quintela]

We only care about the amount of bytes transferred.  Flushing is done
by the system somewhere else.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index b9580a6c7e..b29e80bdc4 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -748,7 +748,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
 static int block_save_iterate(QEMUFile *f, void *opaque)
 {
     int ret;
-    uint64_t last_bytes = qemu_file_transferred(f);
+    uint64_t last_bytes = qemu_file_transferred_noflush(f);
 
     trace_migration_block_save("iterate", block_mig_state.submitted,
                                block_mig_state.transferred);
@@ -800,7 +800,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
     }
 
     qemu_put_be64(f, BLK_MIG_FLAG_EOS);
-    uint64_t delta_bytes = qemu_file_transferred(f) - last_bytes;
+    uint64_t delta_bytes = qemu_file_transferred_noflush(f) - last_bytes;
     return (delta_bytes > 0);
 }
 
-- 
2.40.1

[PATCH v2 04/20] qemu-file: We only call qemu_file_transferred_* on the sending side

[Juan Quintela]

Remove the increase in qemu_file_fill_buffer() and add asserts to
qemu_file_transferred* functions.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 7a5c1a5e32..be3dab85cb 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -342,7 +342,6 @@ static ssize_t coroutine_mixed_fn qemu_fill_buffer(QEMUFile *f)
 
     if (len > 0) {
         f->buf_size += len;
-        f->total_transferred += len;
     } else if (len == 0) {
         qemu_file_set_error_obj(f, -EIO, local_error);
     } else {
@@ -632,6 +631,8 @@ uint64_t qemu_file_transferred_noflush(QEMUFile *f)
     uint64_t ret = f->total_transferred;
     int i;
 
+    g_assert(qemu_file_is_writable(f));
+
     for (i = 0; i < f->iovcnt; i++) {
         ret += f->iov[i].iov_len;
     }
@@ -641,6 +642,7 @@ uint64_t qemu_file_transferred_noflush(QEMUFile *f)
 
 uint64_t qemu_file_transferred(QEMUFile *f)
 {
+    g_assert(qemu_file_is_writable(f));
     qemu_fflush(f);
     return f->total_transferred;
 }
-- 
2.40.1

[PATCH v2 05/20] qemu_file: Use a stat64 for qemu_file_transferred

[Juan Quintela]

This way we can read it from any thread.
I checked that it gives the same value than the current one.  We never
use to qemu_files at the same time.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.h | 4 ++++
 migration/qemu-file.c       | 5 +++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index 2358caad63..b7795e7914 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -81,6 +81,10 @@ typedef struct {
      * Number of bytes sent during precopy stage.
      */
     Stat64 precopy_bytes;
+    /*
+     * Number of bytes transferred with QEMUFile.
+     */
+    Stat64 qemu_file_transferred;
     /*
      * Amount of transferred data at the start of current cycle.
      */
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index be3dab85cb..eb0497e532 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -288,6 +288,7 @@ void qemu_fflush(QEMUFile *f)
         } else {
             uint64_t size = iov_size(f->iov, f->iovcnt);
             f->total_transferred += size;
+            stat64_add(&mig_stats.qemu_file_transferred, size);
         }
 
         qemu_iovec_release_ram(f);
@@ -628,7 +629,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
 
 uint64_t qemu_file_transferred_noflush(QEMUFile *f)
 {
-    uint64_t ret = f->total_transferred;
+    uint64_t ret = stat64_get(&mig_stats.qemu_file_transferred);
     int i;
 
     g_assert(qemu_file_is_writable(f));
@@ -644,7 +645,7 @@ uint64_t qemu_file_transferred(QEMUFile *f)
 {
     g_assert(qemu_file_is_writable(f));
     qemu_fflush(f);
-    return f->total_transferred;
+    return stat64_get(&mig_stats.qemu_file_transferred);
 }
 
 void qemu_put_be16(QEMUFile *f, unsigned int v)
-- 
2.40.1

[PATCH v2 06/20] qemu_file: total_transferred is not used anymore

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index eb0497e532..6b6deea19b 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -41,9 +41,6 @@ struct QEMUFile {
     QIOChannel *ioc;
     bool is_writable;
 
-    /* The sum of bytes transferred on the wire */
-    uint64_t total_transferred;
-
     int buf_index;
     int buf_size; /* 0 when writing */
     uint8_t buf[IO_BUF_SIZE];
@@ -287,7 +284,6 @@ void qemu_fflush(QEMUFile *f)
             qemu_file_set_error_obj(f, -EIO, local_error);
         } else {
             uint64_t size = iov_size(f->iov, f->iovcnt);
-            f->total_transferred += size;
             stat64_add(&mig_stats.qemu_file_transferred, size);
         }
 
-- 
2.40.1

[PATCH v2 07/20] migration: Use the number of transferred bytes directly

[Juan Quintela]

We only use migration_transferred_bytes() to calculate the rate_limit,
for that we don't need to flush whatever is on the qemu_file buffer.
Remember that the buffer is really small (normal case is 32K if we use
iov's can be 64 * TARGET_PAGE_SIZE), so this is not relevant to
calculations.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 79eea8d865..1696185694 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -62,7 +62,7 @@ uint64_t migration_transferred_bytes(QEMUFile *f)
 {
     uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
     uint64_t rdma = stat64_get(&mig_stats.rdma_bytes);
-    uint64_t qemu_file = qemu_file_transferred(f);
+    uint64_t qemu_file = stat64_get(&mig_stats.qemu_file_transferred);
 
     trace_migration_transferred_bytes(qemu_file, multifd, rdma);
     return qemu_file + multifd + rdma;
-- 
2.40.1

[PATCH v2 08/20] qemu_file: Remove unused qemu_file_transferred()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.h | 18 ------------------
 migration/qemu-file.c |  7 -------
 2 files changed, 25 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 21c3f34b84..b4fb872018 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -33,24 +33,6 @@ QEMUFile *qemu_file_new_input(QIOChannel *ioc);
 QEMUFile *qemu_file_new_output(QIOChannel *ioc);
 int qemu_fclose(QEMUFile *f);
 
-/*
- * qemu_file_transferred:
- *
- * Report the total number of bytes transferred with
- * this file.
- *
- * For writable files, any pending buffers will be
- * flushed, so the reported value will be equal to
- * the number of bytes transferred on the wire.
- *
- * For readable files, the reported value will be
- * equal to the number of bytes transferred on the
- * wire.
- *
- * Returns: the total bytes transferred
- */
-uint64_t qemu_file_transferred(QEMUFile *f);
-
 /*
  * qemu_file_transferred_noflush:
  *
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 6b6deea19b..1e6fafc245 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -637,13 +637,6 @@ uint64_t qemu_file_transferred_noflush(QEMUFile *f)
     return ret;
 }
 
-uint64_t qemu_file_transferred(QEMUFile *f)
-{
-    g_assert(qemu_file_is_writable(f));
-    qemu_fflush(f);
-    return stat64_get(&mig_stats.qemu_file_transferred);
-}
-
 void qemu_put_be16(QEMUFile *f, unsigned int v)
 {
     qemu_put_byte(f, v >> 8);
-- 
2.40.1

[PATCH v2 09/20] qemu-file: Remove _noflush from qemu_file_transferred_noflush()

[Juan Quintela]

qemu_file_transferred() don't exist anymore, so we can reuse the name.

Signed-off-by: Juan Quintela <quintela@redhat.com>

---

v2: Update the documentation (thanks fabiano)
---
 migration/qemu-file.h | 9 ++++-----
 migration/block.c     | 4 ++--
 migration/qemu-file.c | 2 +-
 migration/savevm.c    | 6 +++---
 migration/vmstate.c   | 4 ++--
 5 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index b4fb872018..507f3bede4 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -34,15 +34,14 @@ QEMUFile *qemu_file_new_output(QIOChannel *ioc);
 int qemu_fclose(QEMUFile *f);
 
 /*
- * qemu_file_transferred_noflush:
+ * qemu_file_transferred:
  *
- * As qemu_file_transferred except for writable files, where no flush
- * is performed and the reported amount will include the size of any
- * queued buffers, on top of the amount actually transferred.
+ * No flush is performed and the reported amount will include the size
+ * of any queued buffers, on top of the amount actually transferred.
  *
  * Returns: the total bytes transferred and queued
  */
-uint64_t qemu_file_transferred_noflush(QEMUFile *f);
+uint64_t qemu_file_transferred(QEMUFile *f);
 
 /*
  * put_buffer without copying the buffer.
diff --git a/migration/block.c b/migration/block.c
index b29e80bdc4..b9580a6c7e 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -748,7 +748,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
 static int block_save_iterate(QEMUFile *f, void *opaque)
 {
     int ret;
-    uint64_t last_bytes = qemu_file_transferred_noflush(f);
+    uint64_t last_bytes = qemu_file_transferred(f);
 
     trace_migration_block_save("iterate", block_mig_state.submitted,
                                block_mig_state.transferred);
@@ -800,7 +800,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
     }
 
     qemu_put_be64(f, BLK_MIG_FLAG_EOS);
-    uint64_t delta_bytes = qemu_file_transferred_noflush(f) - last_bytes;
+    uint64_t delta_bytes = qemu_file_transferred(f) - last_bytes;
     return (delta_bytes > 0);
 }
 
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 1e6fafc245..94dd6d7fdd 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -623,7 +623,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
     return result;
 }
 
-uint64_t qemu_file_transferred_noflush(QEMUFile *f)
+uint64_t qemu_file_transferred(QEMUFile *f)
 {
     uint64_t ret = stat64_get(&mig_stats.qemu_file_transferred);
     int i;
diff --git a/migration/savevm.c b/migration/savevm.c
index b2199d1039..751b4a1e31 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -927,9 +927,9 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
 static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
                                    JSONWriter *vmdesc)
 {
-    uint64_t old_offset = qemu_file_transferred_noflush(f);
+    uint64_t old_offset = qemu_file_transferred(f);
     se->ops->save_state(f, se->opaque);
-    uint64_t size = qemu_file_transferred_noflush(f) - old_offset;
+    uint64_t size = qemu_file_transferred(f) - old_offset;
 
     if (vmdesc) {
         json_writer_int64(vmdesc, "size", size);
@@ -2952,7 +2952,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
         goto the_end;
     }
     ret = qemu_savevm_state(f, errp);
-    vm_state_size = qemu_file_transferred_noflush(f);
+    vm_state_size = qemu_file_transferred(f);
     ret2 = qemu_fclose(f);
     if (ret < 0) {
         goto the_end;
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 31842c3afb..fd8035ea32 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -361,7 +361,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 void *curr_elem = first_elem + size * i;
 
                 vmsd_desc_field_start(vmsd, vmdesc_loop, field, i, n_elems);
-                old_offset = qemu_file_transferred_noflush(f);
+                old_offset = qemu_file_transferred(f);
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
                     assert(curr_elem);
                     curr_elem = *(void **)curr_elem;
@@ -391,7 +391,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                     return ret;
                 }
 
-                written_bytes = qemu_file_transferred_noflush(f) - old_offset;
+                written_bytes = qemu_file_transferred(f) - old_offset;
                 vmsd_desc_field_end(vmsd, vmdesc_loop, field, written_bytes, i);
 
                 /* Compressed arrays only care about the first element */
-- 
2.40.1

[PATCH v2 10/20] migration: migration_transferred_bytes() don't need the QEMUFile

[Juan Quintela]

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.h | 4 +---
 migration/migration-stats.c | 6 +++---
 migration/migration.c       | 6 +++---
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index b7795e7914..e3863bf9bb 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -137,11 +137,9 @@ void migration_rate_set(uint64_t new_rate);
 /**
  * migration_transferred_bytes: Return number of bytes transferred
  *
- * @f: QEMUFile used for main migration channel
- *
  * Returns how many bytes have we transferred since the beginning of
  * the migration.  It accounts for bytes sent through any migration
  * channel, multifd, qemu_file, rdma, ....
  */
-uint64_t migration_transferred_bytes(QEMUFile *f);
+uint64_t migration_transferred_bytes(void);
 #endif
diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 1696185694..a5f3fcc5ae 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -25,7 +25,7 @@ bool migration_rate_exceeded(QEMUFile *f)
     }
 
     uint64_t rate_limit_start = stat64_get(&mig_stats.rate_limit_start);
-    uint64_t rate_limit_current = migration_transferred_bytes(f);
+    uint64_t rate_limit_current = migration_transferred_bytes();
     uint64_t rate_limit_used = rate_limit_current - rate_limit_start;
     uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max);
 
@@ -55,10 +55,10 @@ void migration_rate_set(uint64_t limit)
 
 void migration_rate_reset(QEMUFile *f)
 {
-    stat64_set(&mig_stats.rate_limit_start, migration_transferred_bytes(f));
+    stat64_set(&mig_stats.rate_limit_start, migration_transferred_bytes());
 }
 
-uint64_t migration_transferred_bytes(QEMUFile *f)
+uint64_t migration_transferred_bytes(void)
 {
     uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
     uint64_t rdma = stat64_get(&mig_stats.rdma_bytes);
diff --git a/migration/migration.c b/migration/migration.c
index 31d30a2ac5..24e55c5bf3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2628,7 +2628,7 @@ static MigThrError migration_detect_error(MigrationState *s)
 
 static void migration_calculate_complete(MigrationState *s)
 {
-    uint64_t bytes = migration_transferred_bytes(s->to_dst_file);
+    uint64_t bytes = migration_transferred_bytes();
     int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     int64_t transfer_time;
 
@@ -2654,7 +2654,7 @@ static void update_iteration_initial_status(MigrationState *s)
      * wrong speed calculation.
      */
     s->iteration_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-    s->iteration_initial_bytes = migration_transferred_bytes(s->to_dst_file);
+    s->iteration_initial_bytes = migration_transferred_bytes();
     s->iteration_initial_pages = ram_get_total_transferred_pages();
 }
 
@@ -2669,7 +2669,7 @@ static void migration_update_counters(MigrationState *s,
         return;
     }
 
-    current_bytes = migration_transferred_bytes(s->to_dst_file);
+    current_bytes = migration_transferred_bytes();
     transferred = current_bytes - s->iteration_initial_bytes;
     time_spent = current_time - s->iteration_start_time;
     bandwidth = (double)transferred / time_spent;
-- 
2.40.1

[PATCH v2 11/20] migration: migration_rate_limit_reset() don't need the QEMUFile

[Juan Quintela]

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.h | 4 +---
 migration/migration-stats.c | 2 +-
 migration/migration.c       | 2 +-
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index e3863bf9bb..68f3939188 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -120,10 +120,8 @@ uint64_t migration_rate_get(void);
  * migration_rate_reset: Reset the rate limit counter.
  *
  * This is called when we know we start a new transfer cycle.
- *
- * @f: QEMUFile used for main migration channel
  */
-void migration_rate_reset(QEMUFile *f);
+void migration_rate_reset(void);
 
 /**
  * migration_rate_set: Set the maximum amount that can be transferred.
diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index a5f3fcc5ae..df541fa904 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -53,7 +53,7 @@ void migration_rate_set(uint64_t limit)
     stat64_set(&mig_stats.rate_limit_max, limit / XFER_LIMIT_RATIO);
 }
 
-void migration_rate_reset(QEMUFile *f)
+void migration_rate_reset(void)
 {
     stat64_set(&mig_stats.rate_limit_start, migration_transferred_bytes());
 }
diff --git a/migration/migration.c b/migration/migration.c
index 24e55c5bf3..ab8254e433 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2693,7 +2693,7 @@ static void migration_update_counters(MigrationState *s,
             stat64_get(&mig_stats.dirty_bytes_last_sync) / bandwidth;
     }
 
-    migration_rate_reset(s->to_dst_file);
+    migration_rate_reset();
 
     update_iteration_initial_status(s);
 
-- 
2.40.1

[PATCH v2 12/20] qemu-file: Simplify qemu_file_get_error()

[Juan Quintela]

If we pass a NULL error is the same that returning dirrectly the value.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 94dd6d7fdd..7200f08ad5 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -209,7 +209,7 @@ void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err)
  */
 int qemu_file_get_error(QEMUFile *f)
 {
-    return qemu_file_get_error_obj(f, NULL);
+    return f->last_error;
 }
 
 /*
-- 
2.40.1

[PATCH v2 13/20] migration: Use migration_transferred_bytes()

[Juan Quintela]

There are only two differnces with the old value:

- the amount of QEMUFile that hasn't yet been flushed.  It can be
  discussed what is more exact, the new or the old one.
- the amount of transferred bytes that we forgot to account for (the
  newer is better, i.e. exact).

Notice that this two values are used to:
a - present to the user
b - calculate the rate_limit

So a few KB here and there is not going to make a difference.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 2 +-
 migration/ram.c       | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index ab8254e433..6ed55d618a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -912,7 +912,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
     size_t page_size = qemu_target_page_size();
 
     info->ram = g_malloc0(sizeof(*info->ram));
-    info->ram->transferred = stat64_get(&mig_stats.transferred);
+    info->ram->transferred = migration_transferred_bytes();
     info->ram->total = ram_bytes_total();
     info->ram->duplicate = stat64_get(&mig_stats.zero_pages);
     /* legacy value.  It is not used anymore */
diff --git a/migration/ram.c b/migration/ram.c
index 87fc504674..0675421f4e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -552,7 +552,7 @@ void mig_throttle_counter_reset(void)
 
     rs->time_last_bitmap_sync = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     rs->num_dirty_pages_period = 0;
-    rs->bytes_xfer_prev = stat64_get(&mig_stats.transferred);
+    rs->bytes_xfer_prev = migration_transferred_bytes();
 }
 
 /**
@@ -988,7 +988,7 @@ static void migration_trigger_throttle(RAMState *rs)
 {
     uint64_t threshold = migrate_throttle_trigger_threshold();
     uint64_t bytes_xfer_period =
-        stat64_get(&mig_stats.transferred) - rs->bytes_xfer_prev;
+        migration_transferred_bytes() - rs->bytes_xfer_prev;
     uint64_t bytes_dirty_period = rs->num_dirty_pages_period * TARGET_PAGE_SIZE;
     uint64_t bytes_dirty_threshold = bytes_xfer_period * threshold / 100;
 
@@ -1051,7 +1051,7 @@ static void migration_bitmap_sync(RAMState *rs, bool last_stage)
         /* reset period counters */
         rs->time_last_bitmap_sync = end_time;
         rs->num_dirty_pages_period = 0;
-        rs->bytes_xfer_prev = stat64_get(&mig_stats.transferred);
+        rs->bytes_xfer_prev = migration_transferred_bytes();
     }
     if (migrate_events()) {
         uint64_t generation = stat64_get(&mig_stats.dirty_sync_count);
-- 
2.40.1

[PATCH v2 14/20] migration: Remove transferred atomic counter

[Juan Quintela]

After last commit, it is a write only variable.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration-stats.h | 4 ----
 migration/multifd.c         | 3 ---
 migration/ram.c             | 1 -
 3 files changed, 8 deletions(-)

diff --git a/migration/migration-stats.h b/migration/migration-stats.h
index 68f3939188..05290ade76 100644
--- a/migration/migration-stats.h
+++ b/migration/migration-stats.h
@@ -97,10 +97,6 @@ typedef struct {
      * Number of bytes sent through RDMA.
      */
     Stat64 rdma_bytes;
-    /*
-     * Total number of bytes transferred.
-     */
-    Stat64 transferred;
     /*
      * Number of pages transferred that were full of zeros.
      */
diff --git a/migration/multifd.c b/migration/multifd.c
index 0bf5958a9c..b297d1cb62 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -188,7 +188,6 @@ static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
         return -1;
     }
     stat64_add(&mig_stats.multifd_bytes, size);
-    stat64_add(&mig_stats.transferred, size);
     return 0;
 }
 
@@ -715,7 +714,6 @@ static void *multifd_send_thread(void *opaque)
                     break;
                 }
                 stat64_add(&mig_stats.multifd_bytes, p->packet_len);
-                stat64_add(&mig_stats.transferred, p->packet_len);
             } else {
                 /* Send header using the same writev call */
                 p->iov[0].iov_len = p->packet_len;
@@ -729,7 +727,6 @@ static void *multifd_send_thread(void *opaque)
             }
 
             stat64_add(&mig_stats.multifd_bytes, p->next_packet_size);
-            stat64_add(&mig_stats.transferred, p->next_packet_size);
             qemu_mutex_lock(&p->mutex);
             p->pending_job--;
             qemu_mutex_unlock(&p->mutex);
diff --git a/migration/ram.c b/migration/ram.c
index 0675421f4e..cdf7783d65 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -443,7 +443,6 @@ void ram_transferred_add(uint64_t bytes)
     } else {
         stat64_add(&mig_stats.downtime_bytes, bytes);
     }
-    stat64_add(&mig_stats.transferred, bytes);
 }
 
 struct MigrationOps {
-- 
2.40.1

[PATCH v2 15/20] qemu-file: Make qemu_fflush() return errors

[Juan Quintela]

This let us simplify code of this shape.

   qemu_fflush(f);
   int ret = qemu_file_get_error(f);
   if (ret) {
      return ret;
   }

into:

   int ret = qemu_fflush(f);
   if (ret) {
      return ret;
   }

I updated all callers where there is any error check.
qemu_fclose() don't need to check for f->last_error because
qemu_fflush() returns it at the beggining of the function.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Juan Quintela <quintela@redhat.com>

---

In v2: Now that we call always qemu_fflush() for all files, we can
simplify qemu_fclose()
---
 migration/qemu-file.h |  2 +-
 migration/colo.c      | 11 +++--------
 migration/migration.c |  7 +------
 migration/qemu-file.c | 23 +++++++----------------
 migration/ram.c       | 22 +++++++---------------
 migration/rdma.c      |  4 +---
 migration/savevm.c    |  3 +--
 7 files changed, 21 insertions(+), 51 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 507f3bede4..f2c118f342 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -74,7 +74,7 @@ void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
 void qemu_file_set_error(QEMUFile *f, int ret);
 int qemu_file_shutdown(QEMUFile *f);
 QEMUFile *qemu_file_get_return_path(QEMUFile *f);
-void qemu_fflush(QEMUFile *f);
+int qemu_fflush(QEMUFile *f);
 void qemu_file_set_blocking(QEMUFile *f, bool block);
 int qemu_file_get_to_fd(QEMUFile *f, int fd, size_t size);
 
diff --git a/migration/colo.c b/migration/colo.c
index 72f4f7b37e..4447e34914 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -314,9 +314,7 @@ static void colo_send_message(QEMUFile *f, COLOMessage msg,
         return;
     }
     qemu_put_be32(f, msg);
-    qemu_fflush(f);
-
-    ret = qemu_file_get_error(f);
+    ret = qemu_fflush(f);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Can't send COLO message");
     }
@@ -335,9 +333,7 @@ static void colo_send_message_value(QEMUFile *f, COLOMessage msg,
         return;
     }
     qemu_put_be64(f, value);
-    qemu_fflush(f);
-
-    ret = qemu_file_get_error(f);
+    ret = qemu_fflush(f);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Failed to send value for message:%s",
                          COLOMessage_str(msg));
@@ -483,8 +479,7 @@ static int colo_do_checkpoint_transaction(MigrationState *s,
     }
 
     qemu_put_buffer(s->to_dst_file, bioc->data, bioc->usage);
-    qemu_fflush(s->to_dst_file);
-    ret = qemu_file_get_error(s->to_dst_file);
+    ret = qemu_fflush(s->to_dst_file);
     if (ret < 0) {
         goto out;
     }
diff --git a/migration/migration.c b/migration/migration.c
index 6ed55d618a..0195105c59 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -297,12 +297,7 @@ static int migrate_send_rp_message(MigrationIncomingState *mis,
     qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
     qemu_put_be16(mis->to_src_file, len);
     qemu_put_buffer(mis->to_src_file, data, len);
-    qemu_fflush(mis->to_src_file);
-
-    /* It's possible that qemu file got error during sending */
-    ret = qemu_file_get_error(mis->to_src_file);
-
-    return ret;
+    return qemu_fflush(mis->to_src_file);
 }
 
 /* Request one page from the source VM at the given start address.
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 7200f08ad5..4f9fb1fbd0 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -267,14 +267,14 @@ static void qemu_iovec_release_ram(QEMUFile *f)
  * This will flush all pending data. If data was only partially flushed, it
  * will set an error state.
  */
-void qemu_fflush(QEMUFile *f)
+int qemu_fflush(QEMUFile *f)
 {
     if (!qemu_file_is_writable(f)) {
-        return;
+        return f->last_error;
     }
 
-    if (qemu_file_get_error(f)) {
-        return;
+    if (f->last_error) {
+        return f->last_error;
     }
     if (f->iovcnt > 0) {
         Error *local_error = NULL;
@@ -292,6 +292,7 @@ void qemu_fflush(QEMUFile *f)
 
     f->buf_index = 0;
     f->iovcnt = 0;
+    return f->last_error;
 }
 
 /*
@@ -358,22 +359,12 @@ static ssize_t coroutine_mixed_fn qemu_fill_buffer(QEMUFile *f)
  */
 int qemu_fclose(QEMUFile *f)
 {
-    int ret, ret2;
-    qemu_fflush(f);
-    ret = qemu_file_get_error(f);
-
-    ret2 = qio_channel_close(f->ioc, NULL);
+    int ret = qemu_fflush(f);
+    int ret2 = qio_channel_close(f->ioc, NULL);
     if (ret >= 0) {
         ret = ret2;
     }
     g_clear_pointer(&f->ioc, object_unref);
-
-    /* If any error was spotted before closing, we should report it
-     * instead of the close() return value.
-     */
-    if (f->last_error) {
-        ret = f->last_error;
-    }
     error_free(f->last_error_obj);
     g_free(f);
     trace_qemu_file_fclose();
diff --git a/migration/ram.c b/migration/ram.c
index cdf7783d65..26baa54d98 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -301,17 +301,15 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file,
 
     qemu_put_be64(file, size);
     qemu_put_buffer(file, (const uint8_t *)le_bitmap, size);
+    g_free(le_bitmap);
     /*
      * Mark as an end, in case the middle part is screwed up due to
      * some "mysterious" reason.
      */
     qemu_put_be64(file, RAMBLOCK_RECV_BITMAP_ENDING);
-    qemu_fflush(file);
-
-    g_free(le_bitmap);
-
-    if (qemu_file_get_error(file)) {
-        return qemu_file_get_error(file);
+    int ret = qemu_fflush(file);
+    if (ret) {
+        return ret;
     }
 
     return size + sizeof(size);
@@ -3034,9 +3032,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     }
 
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
-    qemu_fflush(f);
-
-    return 0;
+    return qemu_fflush(f);
 }
 
 /**
@@ -3155,10 +3151,8 @@ out:
         }
 
         qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
-        qemu_fflush(f);
         ram_transferred_add(8);
-
-        ret = qemu_file_get_error(f);
+        ret = qemu_fflush(f);
     }
     if (ret < 0) {
         return ret;
@@ -3235,9 +3229,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
         qemu_put_be64(f, RAM_SAVE_FLAG_MULTIFD_FLUSH);
     }
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
-    qemu_fflush(f);
-
-    return 0;
+    return qemu_fflush(f);
 }
 
 static void ram_state_pending_estimate(void *opaque, uint64_t *must_precopy,
diff --git a/migration/rdma.c b/migration/rdma.c
index f912a31b45..3ef35fc635 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3886,9 +3886,7 @@ int rdma_registration_start(QEMUFile *f, uint64_t flags)
 
     trace_rdma_registration_start(flags);
     qemu_put_be64(f, RAM_SAVE_FLAG_HOOK);
-    qemu_fflush(f);
-
-    return 0;
+    return qemu_fflush(f);
 }
 
 /*
diff --git a/migration/savevm.c b/migration/savevm.c
index 751b4a1e31..1dc46b0967 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1536,8 +1536,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
     }
 
 flush:
-    qemu_fflush(f);
-    return 0;
+    return qemu_fflush(f);
 }
 
 /* Give an estimate of the amount left to be transferred,
-- 
2.40.1

[PATCH v2 16/20] migration/rdma: Split qemu_fopen_rdma() into input/output functions

[Juan Quintela]

This is how everything else in QEMUFile is structured.
As a bonus they are three less lines of code.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/rdma.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 3ef35fc635..606837bd45 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -4046,25 +4046,22 @@ static void qio_channel_rdma_register_types(void)
 
 type_init(qio_channel_rdma_register_types);
 
-static QEMUFile *qemu_fopen_rdma(RDMAContext *rdma, const char *mode)
+static QEMUFile *rdma_new_input(RDMAContext *rdma)
 {
-    QIOChannelRDMA *rioc;
+    QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(object_new(TYPE_QIO_CHANNEL_RDMA));
+    rioc->file = qemu_file_new_input(QIO_CHANNEL(rioc));
+    rioc->rdmain = rdma;
+    rioc->rdmaout = rdma->return_path;
 
-    if (qemu_file_mode_is_not_valid(mode)) {
-        return NULL;
-    }
+    return rioc->file;
+}
 
-    rioc = QIO_CHANNEL_RDMA(object_new(TYPE_QIO_CHANNEL_RDMA));
-
-    if (mode[0] == 'w') {
-        rioc->file = qemu_file_new_output(QIO_CHANNEL(rioc));
-        rioc->rdmaout = rdma;
-        rioc->rdmain = rdma->return_path;
-    } else {
-        rioc->file = qemu_file_new_input(QIO_CHANNEL(rioc));
-        rioc->rdmain = rdma;
-        rioc->rdmaout = rdma->return_path;
-    }
+static QEMUFile *rdma_new_output(RDMAContext *rdma)
+{
+    QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(object_new(TYPE_QIO_CHANNEL_RDMA));
+    rioc->file = qemu_file_new_output(QIO_CHANNEL(rioc));
+    rioc->rdmaout = rdma;
+    rioc->rdmain = rdma->return_path;
 
     return rioc->file;
 }
@@ -4090,9 +4087,9 @@ static void rdma_accept_incoming_migration(void *opaque)
         return;
     }
 
-    f = qemu_fopen_rdma(rdma, "rb");
+    f = rdma_new_input(rdma);
     if (f == NULL) {
-        fprintf(stderr, "RDMA ERROR: could not qemu_fopen_rdma\n");
+        fprintf(stderr, "RDMA ERROR: could not open RDMA for input\n");
         qemu_rdma_cleanup(rdma);
         return;
     }
@@ -4217,7 +4214,7 @@ void rdma_start_outgoing_migration(void *opaque,
     trace_rdma_start_outgoing_migration_after_rdma_connect();
 
     s->rdma_migration = true;
-    s->to_dst_file = qemu_fopen_rdma(rdma, "wb");
+    s->to_dst_file = rdma_new_output(rdma);
     migrate_fd_connect(s, NULL);
     return;
 return_path_err:
-- 
2.40.1

[PATCH v2 17/20] qemu-file: Remove unused qemu_file_mode_is_not_valid()

[Juan Quintela]

It is not used after previous commit.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.h |  1 -
 migration/qemu-file.c | 12 ------------
 2 files changed, 13 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index f2c118f342..4d5b999fa8 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -49,7 +49,6 @@ uint64_t qemu_file_transferred(QEMUFile *f);
  */
 void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
                            bool may_free);
-bool qemu_file_mode_is_not_valid(const char *mode);
 bool qemu_file_is_writable(QEMUFile *f);
 
 #include "migration/qemu-file-types.h"
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 4f9fb1fbd0..76f10fe5c4 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -99,18 +99,6 @@ int qemu_file_shutdown(QEMUFile *f)
     return ret;
 }
 
-bool qemu_file_mode_is_not_valid(const char *mode)
-{
-    if (mode == NULL ||
-        (mode[0] != 'r' && mode[0] != 'w') ||
-        mode[1] != 'b' || mode[2] != 0) {
-        fprintf(stderr, "qemu_fopen: Argument validity check failed\n");
-        return true;
-    }
-
-    return false;
-}
-
 static QEMUFile *qemu_file_new_impl(QIOChannel *ioc, bool is_writable)
 {
     QEMUFile *f;
-- 
2.40.1

[PATCH v2 18/20] qemu_file: Make qemu_file_is_writable() static

[Juan Quintela]

It is not used outside of qemu_file, and it shouldn't.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.h | 1 -
 migration/qemu-file.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 4d5b999fa8..e3dba9f385 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -49,7 +49,6 @@ uint64_t qemu_file_transferred(QEMUFile *f);
  */
 void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
                            bool may_free);
-bool qemu_file_is_writable(QEMUFile *f);
 
 #include "migration/qemu-file-types.h"
 
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 76f10fe5c4..01616d159c 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -208,7 +208,7 @@ void qemu_file_set_error(QEMUFile *f, int ret)
     qemu_file_set_error_obj(f, ret, NULL);
 }
 
-bool qemu_file_is_writable(QEMUFile *f)
+static bool qemu_file_is_writable(QEMUFile *f)
 {
     return f->is_writable;
 }
-- 
2.40.1

[PATCH v2 19/20] qemu-file: Simplify qemu_file_shutdown()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 01616d159c..e4923ab3a0 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -62,8 +62,6 @@ struct QEMUFile {
  */
 int qemu_file_shutdown(QEMUFile *f)
 {
-    int ret = 0;
-
     /*
      * We must set qemufile error before the real shutdown(), otherwise
      * there can be a race window where we thought IO all went though
@@ -93,10 +91,10 @@ int qemu_file_shutdown(QEMUFile *f)
     }
 
     if (qio_channel_shutdown(f->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL) < 0) {
-        ret = -EIO;
+        return -EIO;
     }
 
-    return ret;
+    return 0;
 }
 
 static QEMUFile *qemu_file_new_impl(QIOChannel *ioc, bool is_writable)
-- 
2.40.1

[PATCH v2 20/20] qemu-file: Make qemu_file_get_error_obj() static

[Juan Quintela]

It was not used outside of qemu_file.c anyways.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/qemu-file.h | 1 -
 migration/qemu-file.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index e3dba9f385..1774116f79 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -66,7 +66,6 @@ bool qemu_file_buffer_empty(QEMUFile *file);
  */
 int coroutine_mixed_fn qemu_peek_byte(QEMUFile *f, int offset);
 void qemu_file_skip(QEMUFile *f, int size);
-int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
 int qemu_file_get_error_obj_any(QEMUFile *f1, QEMUFile *f2, Error **errp);
 void qemu_file_set_error_obj(QEMUFile *f, int ret, Error *err);
 void qemu_file_set_error(QEMUFile *f, int ret);
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index e4923ab3a0..c06a7bed07 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -138,7 +138,7 @@ QEMUFile *qemu_file_new_input(QIOChannel *ioc)
  * is not 0.
  *
  */
-int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
+static int qemu_file_get_error_obj(QEMUFile *f, Error **errp)
 {
     if (errp) {
         *errp = f->last_error_obj ? error_copy(f->last_error_obj) : NULL;
-- 
2.40.1

Re: [PATCH v2 00/20] Next round of migration atomic counters

[Fiona Ebner]

Am 30.05.23 um 20:39 schrieb Juan Quintela:
> Hi
> 
> On this v2:
> 
> - dropped qemu_fflush() assert for read only files (make Richard
>   happy)
> 
> - Update documentation for qemu_file_transferred (make Fabiano happy)
> 
> - migration/rdma: Remove qemu_fopen_rdma() and make it look like
>   everything else
> 
> - Simplify a couple of qemu-file functions, and unexport the ones that
>   are not used outside of qemu-file.c
> 
> - Added Reviewed-by comments.
> 
> Please review.
> 
Hi,
I don't notice any performance regression with snapshots anymore with
this series :)

Note that I had to apply your PULL patches from yesterday too, to be
able to apply this series:
https://lists.nongnu.org/archive/html/qemu-devel/2023-05/msg07361.html

Best Regards,
Fiona

Re: [PATCH v2 00/20] Next round of migration atomic counters

[Juan Quintela]

Fiona Ebner <f.ebner@proxmox.com> wrote:
> Am 30.05.23 um 20:39 schrieb Juan Quintela:
>> Hi
>> 
>> On this v2:
>> 
>> - dropped qemu_fflush() assert for read only files (make Richard
>>   happy)
>> 
>> - Update documentation for qemu_file_transferred (make Fabiano happy)
>> 
>> - migration/rdma: Remove qemu_fopen_rdma() and make it look like
>>   everything else
>> 
>> - Simplify a couple of qemu-file functions, and unexport the ones that
>>   are not used outside of qemu-file.c
>> 
>> - Added Reviewed-by comments.
>> 
>> Please review.
>> 
> Hi,
> I don't notice any performance regression with snapshots anymore with
> this series :)
>
> Note that I had to apply your PULL patches from yesterday too, to be
> able to apply this series:
> https://lists.nongnu.org/archive/html/qemu-devel/2023-05/msg07361.html

I know, thanks.

Later, Juan.

Re: [PATCH v2 04/20] qemu-file: We only call qemu_file_transferred_* on the sending side

[Peter Xu]

On Tue, May 30, 2023 at 08:39:25PM +0200, Juan Quintela wrote:
> Remove the increase in qemu_file_fill_buffer() and add asserts to
> qemu_file_transferred* functions.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

The read side accounting does look a bit weird and never caught my notice..

Maybe worth also touching the document of QEMUFile::total_transferred to
clarify what it accounts?

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

Though when I'm looking at the counters (didn't follow every single recent
patch on this..), I found that now reading transferred value is actually
more expensive - qemu_file_transferred() needs flushing, even if for the
fast version, qemu_file_transferred_fast() loops over all possible iovs,
which can be as large as MAX_IOV_SIZE==64.

To be explicit, I _think_ for each guest page we now need to flush...

  ram_save_iterate
    migration_rate_exceeded
      migration_transferred_bytes
        qemu_file_transferred

I hope I'm wrong..

Does it mean that perhaps we simply need "sent and put into send buffer"
more than "what really got transferred"?  So I start to wonder what's the
origianl purpose of this change, and which one is better..

-- 
Peter Xu

Re: [PATCH v2 05/20] qemu_file: Use a stat64 for qemu_file_transferred

[Peter Xu]

On Tue, May 30, 2023 at 08:39:26PM +0200, Juan Quintela wrote:
> This way we can read it from any thread.
> I checked that it gives the same value than the current one.  We never
> use to qemu_files at the same time.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

The follow up patch may be better to be squashed or it's very confusing..

Why do we need to convert mostly everything into atomics?  Is it modified
outside migration thread?

AFAIR atomic ops are still expensive, and will get more expensive on larger
hosts, IOW it'll be good for us to keep non-atomic when possible (and
that's why when I changed some counters in postcopy work I only changed the
limited set because then the rest are still accessed in 1 single thread and
keep running fast).

> ---
>  migration/migration-stats.h | 4 ++++
>  migration/qemu-file.c       | 5 +++--
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index 2358caad63..b7795e7914 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -81,6 +81,10 @@ typedef struct {
>       * Number of bytes sent during precopy stage.
>       */
>      Stat64 precopy_bytes;
> +    /*
> +     * Number of bytes transferred with QEMUFile.
> +     */
> +    Stat64 qemu_file_transferred;
>      /*
>       * Amount of transferred data at the start of current cycle.
>       */
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index be3dab85cb..eb0497e532 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -288,6 +288,7 @@ void qemu_fflush(QEMUFile *f)
>          } else {
>              uint64_t size = iov_size(f->iov, f->iovcnt);
>              f->total_transferred += size;
> +            stat64_add(&mig_stats.qemu_file_transferred, size);
>          }
>  
>          qemu_iovec_release_ram(f);
> @@ -628,7 +629,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
>  
>  uint64_t qemu_file_transferred_noflush(QEMUFile *f)
>  {
> -    uint64_t ret = f->total_transferred;
> +    uint64_t ret = stat64_get(&mig_stats.qemu_file_transferred);
>      int i;
>  
>      g_assert(qemu_file_is_writable(f));
> @@ -644,7 +645,7 @@ uint64_t qemu_file_transferred(QEMUFile *f)
>  {
>      g_assert(qemu_file_is_writable(f));
>      qemu_fflush(f);
> -    return f->total_transferred;
> +    return stat64_get(&mig_stats.qemu_file_transferred);
>  }
>  
>  void qemu_put_be16(QEMUFile *f, unsigned int v)
> -- 
> 2.40.1
> 

-- 
Peter Xu

Re: [PATCH v2 04/20] qemu-file: We only call qemu_file_transferred_* on the sending side

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Tue, May 30, 2023 at 08:39:25PM +0200, Juan Quintela wrote:
>> Remove the increase in qemu_file_fill_buffer() and add asserts to
>> qemu_file_transferred* functions.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> The read side accounting does look a bit weird and never caught my notice..
>
> Maybe worth also touching the document of QEMUFile::total_transferred to
> clarify what it accounts?
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Though when I'm looking at the counters (didn't follow every single recent
> patch on this..), I found that now reading transferred value is actually
> more expensive - qemu_file_transferred() needs flushing, even if for the
> fast version, qemu_file_transferred_fast() loops over all possible iovs,
> which can be as large as MAX_IOV_SIZE==64.
>
> To be explicit, I _think_ for each guest page we now need to flush...
>
>   ram_save_iterate
>     migration_rate_exceeded
>       migration_transferred_bytes
>         qemu_file_transferred
>
> I hope I'm wrong..

See patch 7:

diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 79eea8d865..1696185694 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -62,7 +62,7 @@ uint64_t migration_transferred_bytes(QEMUFile *f)
 {
     uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
     uint64_t rdma = stat64_get(&mig_stats.rdma_bytes);
-    uint64_t qemu_file = qemu_file_transferred(f);
+    uint64_t qemu_file = stat64_get(&mig_stats.qemu_file_transferred);
 
     trace_migration_transferred_bytes(qemu_file, multifd, rdma);
     return qemu_file + multifd + rdma;


> Does it mean that perhaps we simply need "sent and put into send buffer"
> more than "what really got transferred"?  So I start to wonder what's the
> origianl purpose of this change, and which one is better..

That is basically what patch 5 and 6 do O:-)

Problem is arriving to something that is bisectable (for correctness)
and is easy to review.

And yes, my choices can be different from the ones tat you do.

The other reason for the small patches is that:
a - sometimes I found a different test where things broke, and have to
    bisect
b - small patches are much easier to rebase (that I am doing a lot)

Later, Juan.

Re: [PATCH v2 05/20] qemu_file: Use a stat64 for qemu_file_transferred

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Tue, May 30, 2023 at 08:39:26PM +0200, Juan Quintela wrote:
>> This way we can read it from any thread.
>> I checked that it gives the same value than the current one.  We never
>> use to qemu_files at the same time.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> The follow up patch may be better to be squashed or it's very confusing..

As said before, there used to be a nice:

if (old_counter != new_counter)
   printf(old_counter and new counter)

> Why do we need to convert mostly everything into atomics?  Is it modified
> outside migration thread?

Because we have four users of this stuff:

- io thread: needs it for info migrate basically
- migration_thread: needs it to know when to stop.  qemu_file is
  supposed to only happen here.
- ... until someone called peter decided to create another qemu file for
  preempt.  Current code don't account for this.
- multifd bytes is a completelly diffrent story, that is why it use its
  own atomics.

After this series go in, basically we only update one atomic for each
write (ok two yet, downtime/precopy/postocpy_bytes are still not
integrated).

But there used to be three:
- transferred
- multifd_bytes (for multifd pages)
- downtime/-precopy/postocpy

And we are back to only one (now we use transferred or multifd, never both).

> AFAIR atomic ops are still expensive, and will get more expensive on larger
> hosts,

If you care for performance, you are using multifd.
one atomic every 64 pages is kind ok.

For rate limiting, we do it 10 times a second, and:
a - it shouldn't matter with so few times
b - it is not a regression, we were already updating some
c - correct vs fast any day

And the other big improvement is that after this series, you can use any
counter on any thread.  No need to contorsions to update the rate_limit
that we used to have.

> IOW it'll be good for us to keep non-atomic when possible (and
> that's why when I changed some counters in postcopy work I only changed the
> limited set because then the rest are still accessed in 1 single thread and
> keep running fast).

void ram_transferred_add(uint64_t bytes)
{
    if (runstate_is_running()) {
        stat64_add(&mig_stats.precopy_bytes, bytes);
    } else if (migration_in_postcopy()) {
        stat64_add(&mig_stats.postcopy_bytes, bytes);
    } else {
        stat64_add(&mig_stats.downtime_bytes, bytes);
    }
    stat64_add(&mig_stats.transferred, bytes);
}

That is used everywhere, and I am dropping this to a single atomic.

Later, Juan.

Re: [PATCH v2 04/20] qemu-file: We only call qemu_file_transferred_* on the sending side

[Peter Xu]

On Tue, Jun 13, 2023 at 06:02:05PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Tue, May 30, 2023 at 08:39:25PM +0200, Juan Quintela wrote:
> >> Remove the increase in qemu_file_fill_buffer() and add asserts to
> >> qemu_file_transferred* functions.
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >
> > The read side accounting does look a bit weird and never caught my notice..
> >
> > Maybe worth also touching the document of QEMUFile::total_transferred to
> > clarify what it accounts?
> >
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> >
> > Though when I'm looking at the counters (didn't follow every single recent
> > patch on this..), I found that now reading transferred value is actually
> > more expensive - qemu_file_transferred() needs flushing, even if for the
> > fast version, qemu_file_transferred_fast() loops over all possible iovs,
> > which can be as large as MAX_IOV_SIZE==64.
> >
> > To be explicit, I _think_ for each guest page we now need to flush...
> >
> >   ram_save_iterate
> >     migration_rate_exceeded
> >       migration_transferred_bytes
> >         qemu_file_transferred
> >
> > I hope I'm wrong..
> 
> See patch 7:
> 
> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
> index 79eea8d865..1696185694 100644
> --- a/migration/migration-stats.c
> +++ b/migration/migration-stats.c
> @@ -62,7 +62,7 @@ uint64_t migration_transferred_bytes(QEMUFile *f)
>  {
>      uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
>      uint64_t rdma = stat64_get(&mig_stats.rdma_bytes);
> -    uint64_t qemu_file = qemu_file_transferred(f);
> +    uint64_t qemu_file = stat64_get(&mig_stats.qemu_file_transferred);
>  
>      trace_migration_transferred_bytes(qemu_file, multifd, rdma);
>      return qemu_file + multifd + rdma;

If this is a known regression, should we make a first patchset fix it and
make it higher priority to merge?

It seems this is even not mentioned in the cover letter.. while IMHO this
is the most important bit to have in it..

> 
> 
> > Does it mean that perhaps we simply need "sent and put into send buffer"
> > more than "what really got transferred"?  So I start to wonder what's the
> > origianl purpose of this change, and which one is better..
> 
> That is basically what patch 5 and 6 do O:-)
> 
> Problem is arriving to something that is bisectable (for correctness)
> and is easy to review.
> 
> And yes, my choices can be different from the ones tat you do.
> 
> The other reason for the small patches is that:
> a - sometimes I found a different test where things broke, and have to
>     bisect
> b - small patches are much easier to rebase (that I am doing a lot)

That's okay.  Thanks,

-- 
Peter Xu

Re: [PATCH v2 06/20] qemu_file: total_transferred is not used anymore

[Peter Xu]

On Tue, May 30, 2023 at 08:39:27PM +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index eb0497e532..6b6deea19b 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -41,9 +41,6 @@ struct QEMUFile {
>      QIOChannel *ioc;
>      bool is_writable;
>  
> -    /* The sum of bytes transferred on the wire */
> -    uint64_t total_transferred;
> -
>      int buf_index;
>      int buf_size; /* 0 when writing */
>      uint8_t buf[IO_BUF_SIZE];
> @@ -287,7 +284,6 @@ void qemu_fflush(QEMUFile *f)
>              qemu_file_set_error_obj(f, -EIO, local_error);
>          } else {
>              uint64_t size = iov_size(f->iov, f->iovcnt);
> -            f->total_transferred += size;

I think this patch is another example why I think sometimes the way patch
is split are pretty much adding more complexity on review...

Here we removed a variable operation but it seems all fine if it's not used
anywhere.  But it also means current code base (before this patch applied)
doesn't make sense already because it contains this useless addition.  So
IMHO it means some previous patch does it just wrong.

I think it means it's caused by a wrong split of patches, then each patch
stops to make much sense as a standalone one.

I can go back and try to find whatever patch on the list that will explain
this.  But it'll also go into git log.  Anyone reads this later will be
confused once again.  Even harder for them to figure out what happened.

Do you think we could reorganize the patches so each of a single patch
explains itself?

The other thing is about priority of patches - I still have ~80 patches
pending reviews on migration only.. Would you think it makes sense we pick
up important ones first and merge them with higher priority?

What I have in mind are:

  - The regression you mentioned on qemu_fflush() when ram save (I hope I
    understand the problem right, though...).

  - The slowness of migration-test.  I'm not sure whether you have anything
    better than either Dan's approach or the switchover-hold flags, I just
    think that seems more important to resolve for us upstream.  We can
    merge Dan's or mine, you can also propose something else, but IMHO that
    seems to be a higher priority?

And whatever else I haven't noticed.  I'll continue reading but I'm sure
you know the best on this..  so I'd really rely on you.

What do you think?

Thanks,

-- 
Peter Xu

Re: [PATCH v2 16/20] migration/rdma: Split qemu_fopen_rdma() into input/output functions

[Peter Xu]

On Tue, May 30, 2023 at 08:39:37PM +0200, Juan Quintela wrote:
> This is how everything else in QEMUFile is structured.
> As a bonus they are three less lines of code.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/rdma.c | 35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 3ef35fc635..606837bd45 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -4046,25 +4046,22 @@ static void qio_channel_rdma_register_types(void)
>  
>  type_init(qio_channel_rdma_register_types);
>  
> -static QEMUFile *qemu_fopen_rdma(RDMAContext *rdma, const char *mode)
> +static QEMUFile *rdma_new_input(RDMAContext *rdma)
>  {
> -    QIOChannelRDMA *rioc;
> +    QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(object_new(TYPE_QIO_CHANNEL_RDMA));
> +    rioc->file = qemu_file_new_input(QIO_CHANNEL(rioc));
> +    rioc->rdmain = rdma;
> +    rioc->rdmaout = rdma->return_path;
>  
> -    if (qemu_file_mode_is_not_valid(mode)) {

We can also drop this function together.  If with that:

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

> -        return NULL;
> -    }
> +    return rioc->file;
> +}
>  
> -    rioc = QIO_CHANNEL_RDMA(object_new(TYPE_QIO_CHANNEL_RDMA));
> -
> -    if (mode[0] == 'w') {
> -        rioc->file = qemu_file_new_output(QIO_CHANNEL(rioc));
> -        rioc->rdmaout = rdma;
> -        rioc->rdmain = rdma->return_path;
> -    } else {
> -        rioc->file = qemu_file_new_input(QIO_CHANNEL(rioc));
> -        rioc->rdmain = rdma;
> -        rioc->rdmaout = rdma->return_path;
> -    }
> +static QEMUFile *rdma_new_output(RDMAContext *rdma)
> +{
> +    QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(object_new(TYPE_QIO_CHANNEL_RDMA));
> +    rioc->file = qemu_file_new_output(QIO_CHANNEL(rioc));
> +    rioc->rdmaout = rdma;
> +    rioc->rdmain = rdma->return_path;
>  
>      return rioc->file;
>  }
> @@ -4090,9 +4087,9 @@ static void rdma_accept_incoming_migration(void *opaque)
>          return;
>      }
>  
> -    f = qemu_fopen_rdma(rdma, "rb");
> +    f = rdma_new_input(rdma);
>      if (f == NULL) {
> -        fprintf(stderr, "RDMA ERROR: could not qemu_fopen_rdma\n");
> +        fprintf(stderr, "RDMA ERROR: could not open RDMA for input\n");
>          qemu_rdma_cleanup(rdma);
>          return;
>      }
> @@ -4217,7 +4214,7 @@ void rdma_start_outgoing_migration(void *opaque,
>      trace_rdma_start_outgoing_migration_after_rdma_connect();
>  
>      s->rdma_migration = true;
> -    s->to_dst_file = qemu_fopen_rdma(rdma, "wb");
> +    s->to_dst_file = rdma_new_output(rdma);
>      migrate_fd_connect(s, NULL);
>      return;
>  return_path_err:
> -- 
> 2.40.1
> 

-- 
Peter Xu

Re: [PATCH v2 18/20] qemu_file: Make qemu_file_is_writable() static

[Peter Xu]

On Tue, May 30, 2023 at 08:39:39PM +0200, Juan Quintela wrote:
> It is not used outside of qemu_file, and it shouldn't.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

-- 
Peter Xu

Re: [PATCH v2 19/20] qemu-file: Simplify qemu_file_shutdown()

[Peter Xu]

On Tue, May 30, 2023 at 08:39:40PM +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

-- 
Peter Xu

Re: [PATCH v2 20/20] qemu-file: Make qemu_file_get_error_obj() static

[Peter Xu]

On Tue, May 30, 2023 at 08:39:41PM +0200, Juan Quintela wrote:
> It was not used outside of qemu_file.c anyways.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

-- 
Peter Xu

Re: [PATCH v2 04/20] qemu-file: We only call qemu_file_transferred_* on the sending side

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Tue, Jun 13, 2023 at 06:02:05PM +0200, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > On Tue, May 30, 2023 at 08:39:25PM +0200, Juan Quintela wrote:
>> >> Remove the increase in qemu_file_fill_buffer() and add asserts to
>> >> qemu_file_transferred* functions.
>> >> 
>> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >
>> > The read side accounting does look a bit weird and never caught my notice..
>> >
>> > Maybe worth also touching the document of QEMUFile::total_transferred to
>> > clarify what it accounts?
>> >
>> > Reviewed-by: Peter Xu <peterx@redhat.com>
>> >
>> > Though when I'm looking at the counters (didn't follow every single recent
>> > patch on this..), I found that now reading transferred value is actually
>> > more expensive - qemu_file_transferred() needs flushing, even if for the
>> > fast version, qemu_file_transferred_fast() loops over all possible iovs,
>> > which can be as large as MAX_IOV_SIZE==64.
>> >
>> > To be explicit, I _think_ for each guest page we now need to flush...
>> >
>> >   ram_save_iterate
>> >     migration_rate_exceeded
>> >       migration_transferred_bytes
>> >         qemu_file_transferred
>> >
>> > I hope I'm wrong..
>> 
>> See patch 7:
>> 
>> diff --git a/migration/migration-stats.c b/migration/migration-stats.c
>> index 79eea8d865..1696185694 100644
>> --- a/migration/migration-stats.c
>> +++ b/migration/migration-stats.c
>> @@ -62,7 +62,7 @@ uint64_t migration_transferred_bytes(QEMUFile *f)
>>  {
>>      uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
>>      uint64_t rdma = stat64_get(&mig_stats.rdma_bytes);
>> -    uint64_t qemu_file = qemu_file_transferred(f);
>> +    uint64_t qemu_file = stat64_get(&mig_stats.qemu_file_transferred);
>>  
>>      trace_migration_transferred_bytes(qemu_file, multifd, rdma);
>>      return qemu_file + multifd + rdma;
>
> If this is a known regression, should we make a first patchset fix it and
> make it higher priority to merge?

This is the simpler way that I have found to arrive from A to B.
The reason why I didn't want to enter the atomic vars (and everybody
before) was because it had so many changing bits.

And here we are, moving to single counters instead of several of them
took something like 200 patches.

> It seems this is even not mentioned in the cover letter.. while IMHO this
> is the most important bit to have in it..

My fault.

I cc'd Fiona on v1, and she confirmed that this fixed her problem.

This is the commit that introduces the slowdown.

commit 813cd61669e45ee6d5db09a83d03df8f0c6eb5d2
Author: Juan Quintela <quintela@redhat.com>
Date:   Mon May 15 21:57:01 2023 +0200

    migration: Use migration_transferred_bytes() to calculate rate_limit
    
    Signed-off-by: Juan Quintela <quintela@redhat.com>
    Reviewed-by: Cédric Le Goater <clg@kaod.org>
    Message-Id: <20230515195709.63843-9-quintela@redhat.com>

Important bits:

-    uint64_t rate_limit_used = stat64_get(&mig_stats.rate_limit_used);
+    uint64_t rate_limit_start = stat64_get(&mig_stats.rate_limit_start);
+    uint64_t rate_limit_current = migration_transferred_bytes(f);
+    uint64_t rate_limit_used = rate_limit_current - rate_limit_start;
     uint64_t rate_limit_max = stat64_get(&mig_stats.rate_limit_max);

We moved from reading an atomic to call qemu_file_transferred(), that
does the iovec dance.

This commit (on this series):

ommit 524072cb5f5ce5605f1171f86ba0879405e4b9b3
Author: Juan Quintela <quintela@redhat.com>
Date:   Mon May 8 12:16:47 2023 +0200

    migration: Use the number of transferred bytes directly
    
    We only use migration_transferred_bytes() to calculate the rate_limit,
    for that we don't need to flush whatever is on the qemu_file buffer.
    Remember that the buffer is really small (normal case is 32K if we use
    iov's can be 64 * TARGET_PAGE_SIZE), so this is not relevant to
    calculations.
    
    Signed-off-by: Juan Quintela <quintela@redhat.com>

diff --git a/migration/migration-stats.c b/migration/migration-stats.c
index 79eea8d865..1696185694 100644
--- a/migration/migration-stats.c
+++ b/migration/migration-stats.c
@@ -62,7 +62,7 @@ uint64_t migration_transferred_bytes(QEMUFile *f)
 {
     uint64_t multifd = stat64_get(&mig_stats.multifd_bytes);
     uint64_t rdma = stat64_get(&mig_stats.rdma_bytes);
-    uint64_t qemu_file = qemu_file_transferred(f);
+    uint64_t qemu_file = stat64_get(&mig_stats.qemu_file_transferred);
 
     trace_migration_transferred_bytes(qemu_file, multifd, rdma);
     return qemu_file + multifd + rdma;

Undoes the damage.

And yes, before you ask, I got this wrong lots of times, have to rebase
and changing order of patches several times O:-)

>> 
>> > Does it mean that perhaps we simply need "sent and put into send buffer"
>> > more than "what really got transferred"?  So I start to wonder what's the
>> > origianl purpose of this change, and which one is better..
>> 
>> That is basically what patch 5 and 6 do O:-)
>> 
>> Problem is arriving to something that is bisectable (for correctness)
>> and is easy to review.
>> 
>> And yes, my choices can be different from the ones tat you do.
>> 
>> The other reason for the small patches is that:
>> a - sometimes I found a different test where things broke, and have to
>>     bisect
>> b - small patches are much easier to rebase (that I am doing a lot)
>
> That's okay.  Thanks,

Later, Juan.

Re: [PATCH v2 06/20] qemu_file: total_transferred is not used anymore

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Tue, May 30, 2023 at 08:39:27PM +0200, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/qemu-file.c | 4 ----
>>  1 file changed, 4 deletions(-)
>> 
>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> index eb0497e532..6b6deea19b 100644
>> --- a/migration/qemu-file.c
>> +++ b/migration/qemu-file.c
>> @@ -41,9 +41,6 @@ struct QEMUFile {
>>      QIOChannel *ioc;
>>      bool is_writable;
>>  
>> -    /* The sum of bytes transferred on the wire */
>> -    uint64_t total_transferred;
>> -
>>      int buf_index;
>>      int buf_size; /* 0 when writing */
>>      uint8_t buf[IO_BUF_SIZE];
>> @@ -287,7 +284,6 @@ void qemu_fflush(QEMUFile *f)
>>              qemu_file_set_error_obj(f, -EIO, local_error);
>>          } else {
>>              uint64_t size = iov_size(f->iov, f->iovcnt);
>> -            f->total_transferred += size;
>
> I think this patch is another example why I think sometimes the way patch
> is split are pretty much adding more complexity on review...

It depends of taste.

You are doing one thing in way1.
Then you find a better way to do it, lets call it way2.

Now we have two options to see how we arrived there.

a- You got any declarations/definition/initializations for way2
b- You write way2 alongside way1
c- You test that both ways give the same result, and you see that they
   give the same result.
d- you remove the way1.

Or you squash the four patches in a single patch.  But then the reviewer
lost the place where one can see why it is the same than the old one.

Sometimes is better the longer way, sometimes is better the short one.

Clearly we don't agree about what is the best way in this case.

> Here we removed a variable operation but it seems all fine if it's not used
> anywhere.  But it also means current code base (before this patch applied)
> doesn't make sense already because it contains this useless addition.  So
> IMHO it means some previous patch does it just wrong.

No.  It is how it is developed.  And being respectful with the
reviewer.  Given it enough information to do a proper review.

During the development of this series, there were lots of:

if (old_counter != new_counter)
   printf("....");

traces were in the several thousand lines long.  If I have to review
that change, I would love any help that writer can give me.  That is why
it is done this way.

> I think it means it's caused by a wrong split of patches, then each patch
> stops to make much sense as a standalone one.

It stops making sense if you want each feature to be a single patch.
Before the patch no feature.  After the patch full feature.  That brings
us to very long patches.

What is easier to review (to do the same)

a - 1 x 1000 lines patch
b - 10 x 100 lines patch

I will go with b any time.  Except if the split is arbitrary.

> I can go back and try to find whatever patch on the list that will explain
> this.  But it'll also go into git log.  Anyone reads this later will be
> confused once again.  Even harder for them to figure out what
> happened.

As said before, I completely disagree here.  And what is worse.  If it
gets wrong, with your approach git bisect will not help as much than
with my appreach.

> Do you think we could reorganize the patches so each of a single patch
> explains itself?

No.  See before.  We go for a very spaguetti code to a much less
spaguety code.

> The other thing is about priority of patches - I still have ~80 patches
> pending reviews on migration only.. Would you think it makes sense we pickg
> up important ones first and merge them with higher priority?

Ok, lets make this clear.
This whole atomic migration counters started because the zero_page
detection in multifd had the counters so wrong that meassuring speed
become impossible.

I haven't yet send the multifd zero pages.  And why was it so
complicated.  Just on top of my memory.

- how much data had we transferred.  Historically we stored that
  information on qemu-file.  But qemu-file can only be read/written from
  the migration thread.  So we went through jumps to be able to update
  that values.

  Current upstream code for compressed multifd assumes that it transfer
  as much data as non compressed one.  Why?  because we don't have an
  easy way to get that value back.  Contorsions that we were trying to
  do:

  https://lore.kernel.org/all/20220802063907.18882-5-quintela@redhat.com/

  To resume, the way that we had to do it was something like:

  - we send a bunch of pages to multifd thread
  - multifd thread send data and returns on the buffer what has written
  - migration thread when reuses a buffer adds the written stuff from
    previous time than the struct was used.

  This was not just problematic from multifd zero pages detection.
  * compression was lying about it
  * zero_copy is doing it wrong (accounting at the time that it does the
    write, not when it knows that it was written).

- rdma: this is even funnier
  * It accounted for zero and normal pages in two places
    https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg07693.html
    (still does, I have to resed that bit)
  * It accounts for imaginary transfers
    https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg07700.html
    Because it has to give the apparence of progress, i.e. that it has
    written something, but it is not true because RDMA is completely
    asynchronous.
  * RDMA and qemu-file were very, very difficult to put appart.
    Remember that RDMA don't send _anything_ through the qemu_file, it
    has its parameter only for the accounting.

- counters:
  qemu_file can only be accessed through Migration thread.
  But each time that you do an info migrate, it is done through the IO
  thread, not the migration thread.  So it was accessing a shared
  variable without any locking.  And putting locking means that we also
  need to lock it on the Migration thread.  So everything that is
  exported to the user needs to be atomic.

- Postcopy preempt
  And here we are, another thread. That uses qemu file, another qemu
  file.  Its access is not racy, because .... we don't account for the
  data sent through the preempt channel.  At all.  Because ... it is
  complicated.

- But we are not happy with this.  We have to calculate the rate limit.
  And for that, we use another counter on the qemu file.  that is
  updated on (almost) the same places that we update the transferred
  counter.  Basically the difference is that multifd don't update the
  transferred counter but update the rate_limit. But RDMA updates both.

- Not happy with this, we decided that this was too complicated and
  added yet another counter.  transferreed.  And atomic one.  You are
  going to ask why.  Well, I am guessing here.  But the problem is that
  when can do info migrate after ending a migration.  And at that time
  qemu-file is gone, so we add another counter instead of storing the
  value of qemu-file.

Should I continue, and search for the patches that changed the things,
or can we agree that this is a complex problem and can't be fixed with
yet another one line?  I spent the best part of a couple of months
trying to fix the problem with one liners, and ended without fixing the
problem after too many one liners.

Ended spending another couple of months writting changing the code
correctly, simplifying the number of counters and giving the same
functionality that was before.  But it took too many patches.

And why it ends with so many patches?
I am glad that you asked.  Because I find a bug.  And I try to fix it.
And then I see that there is another thing that I need to fix to be able
to fix this one.  And another.  And another.

> What I have in mind are:
>
>   - The regression you mentioned on qemu_fflush() when ram save (I hope I
>     understand the problem right, though...).

After the PULL request that I am about to send, we need to get another 4
patches reviewed.

>   - The slowness of migration-test.  I'm not sure whether you have anything
>     better than either Dan's approach

too complex for my taste, and don't get all the speed back.

> or the switchover-hold flags,

I proposed this, but in a different way, will try to send something
before the week ends, sorry for the delay.

> I just
>     think that seems more important to resolve for us upstream.  We can
>     merge Dan's or mine, you can also propose something else, but IMHO that
>     seems to be a higher priority?

> And whatever else I haven't noticed.  I'll continue reading but I'm sure
> you know the best on this..  so I'd really rely on you.

> What do you think?

Thanks, Juan.

Re: [PATCH v2 06/20] qemu_file: total_transferred is not used anymore

[Peter Xu]

On Thu, Jun 22, 2023 at 01:05:49AM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Tue, May 30, 2023 at 08:39:27PM +0200, Juan Quintela wrote:
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  migration/qemu-file.c | 4 ----
> >>  1 file changed, 4 deletions(-)
> >> 
> >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> >> index eb0497e532..6b6deea19b 100644
> >> --- a/migration/qemu-file.c
> >> +++ b/migration/qemu-file.c
> >> @@ -41,9 +41,6 @@ struct QEMUFile {
> >>      QIOChannel *ioc;
> >>      bool is_writable;
> >>  
> >> -    /* The sum of bytes transferred on the wire */
> >> -    uint64_t total_transferred;
> >> -
> >>      int buf_index;
> >>      int buf_size; /* 0 when writing */
> >>      uint8_t buf[IO_BUF_SIZE];
> >> @@ -287,7 +284,6 @@ void qemu_fflush(QEMUFile *f)
> >>              qemu_file_set_error_obj(f, -EIO, local_error);
> >>          } else {
> >>              uint64_t size = iov_size(f->iov, f->iovcnt);
> >> -            f->total_transferred += size;
> >
> > I think this patch is another example why I think sometimes the way patch
> > is split are pretty much adding more complexity on review...
> 
> It depends of taste.
> 
> You are doing one thing in way1.
> Then you find a better way to do it, lets call it way2.
> 
> Now we have two options to see how we arrived there.
> 
> a- You got any declarations/definition/initializations for way2
> b- You write way2 alongside way1
> c- You test that both ways give the same result, and you see that they
>    give the same result.
> d- you remove the way1.
> 
> Or you squash the four patches in a single patch.  But then the reviewer
> lost the place where one can see why it is the same than the old one.

For this patch to remove total_transferred, IMHO as a reviewer it'll be
easier to me if it's put in the same patch where it got replaced.

It might be different if we're going to remove a large chunk of code, but
for this patch it's a few lines of change.

> 
> Sometimes is better the longer way, sometimes is better the short one.
> 
> Clearly we don't agree about what is the best way in this case.
> 
> > Here we removed a variable operation but it seems all fine if it's not used
> > anywhere.  But it also means current code base (before this patch applied)
> > doesn't make sense already because it contains this useless addition.  So
> > IMHO it means some previous patch does it just wrong.
> 
> No.  It is how it is developed.  And being respectful with the
> reviewer.  Given it enough information to do a proper review.

I never doubted about that.  I trust that you were trying to provide the
best for a reviewer and I appreciate that.

> 
> During the development of this series, there were lots of:
> 
> if (old_counter != new_counter)
>    printf("....");

I assume you meant when debugging?  I may not have read all the patches; I
hope we just don't do that if possible in any real git commit.

> 
> traces were in the several thousand lines long.  If I have to review
> that change, I would love any help that writer can give me.  That is why
> it is done this way.

Yeah, I think it's probably that even from reviewers' perspective the need
can be different from individuals.

> 
> > I think it means it's caused by a wrong split of patches, then each patch
> > stops to make much sense as a standalone one.
> 
> It stops making sense if you want each feature to be a single patch.
> Before the patch no feature.  After the patch full feature.  That brings
> us to very long patches.
> 
> What is easier to review (to do the same)
> 
> a - 1 x 1000 lines patch
> b - 10 x 100 lines patch
> 
> I will go with b any time.  Except if the split is arbitrary.

AFAIU, this is a different thing.  I'm never against splitting patch, but
about how to split.  I was never asking for a 1000 LOC patch, right? :)

> 
> > I can go back and try to find whatever patch on the list that will explain
> > this.  But it'll also go into git log.  Anyone reads this later will be
> > confused once again.  Even harder for them to figure out what
> > happened.
> 
> As said before, I completely disagree here.  And what is worse.  If it
> gets wrong, with your approach git bisect will not help as much than
> with my appreach.
> 
> > Do you think we could reorganize the patches so each of a single patch
> > explains itself?
> 
> No.  See before.  We go for a very spaguetti code to a much less
> spaguety code.
> 
> > The other thing is about priority of patches - I still have ~80 patches
> > pending reviews on migration only.. Would you think it makes sense we pickg
> > up important ones first and merge them with higher priority?
> 
> Ok, lets make this clear.
> This whole atomic migration counters started because the zero_page
> detection in multifd had the counters so wrong that meassuring speed
> become impossible.
> 
> I haven't yet send the multifd zero pages.  And why was it so
> complicated.  Just on top of my memory.
> 
> - how much data had we transferred.  Historically we stored that
>   information on qemu-file.  But qemu-file can only be read/written from
>   the migration thread.  So we went through jumps to be able to update
>   that values.
> 
>   Current upstream code for compressed multifd assumes that it transfer
>   as much data as non compressed one.  Why?  because we don't have an
>   easy way to get that value back.  Contorsions that we were trying to
>   do:
> 
>   https://lore.kernel.org/all/20220802063907.18882-5-quintela@redhat.com/
> 
>   To resume, the way that we had to do it was something like:
> 
>   - we send a bunch of pages to multifd thread
>   - multifd thread send data and returns on the buffer what has written
>   - migration thread when reuses a buffer adds the written stuff from
>     previous time than the struct was used.
> 
>   This was not just problematic from multifd zero pages detection.
>   * compression was lying about it
>   * zero_copy is doing it wrong (accounting at the time that it does the
>     write, not when it knows that it was written).
> 
> - rdma: this is even funnier
>   * It accounted for zero and normal pages in two places
>     https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg07693.html
>     (still does, I have to resed that bit)
>   * It accounts for imaginary transfers
>     https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg07700.html
>     Because it has to give the apparence of progress, i.e. that it has
>     written something, but it is not true because RDMA is completely
>     asynchronous.
>   * RDMA and qemu-file were very, very difficult to put appart.
>     Remember that RDMA don't send _anything_ through the qemu_file, it
>     has its parameter only for the accounting.
> 
> - counters:
>   qemu_file can only be accessed through Migration thread.
>   But each time that you do an info migrate, it is done through the IO
>   thread, not the migration thread.  So it was accessing a shared
>   variable without any locking.

Pure readers are IMHO fine as long as there's 1 single writer, and as long
as the reader can read values right even if old (not wrong value;
e.g. 32bit host read over 64 bits vars is a different thing when the var
got updated).

>   And putting locking means that we also need to lock it on the Migration
>   thread.  So everything that is exported to the user needs to be atomic.
> 
> - Postcopy preempt
>   And here we are, another thread. That uses qemu file, another qemu
>   file.  Its access is not racy, because .... we don't account for the
>   data sent through the preempt channel.  At all.  Because ... it is
>   complicated.

Yeah I must confess I didn't pay huge attention on the accounting over
there.. as I want always got the pages sent immediately, ignoring limits
(but agree we should still account those).

> 
> - But we are not happy with this.  We have to calculate the rate limit.
>   And for that, we use another counter on the qemu file.  that is
>   updated on (almost) the same places that we update the transferred
>   counter.  Basically the difference is that multifd don't update the
>   transferred counter but update the rate_limit. But RDMA updates both.
> 
> - Not happy with this, we decided that this was too complicated and
>   added yet another counter.  transferreed.  And atomic one.  You are
>   going to ask why.  Well, I am guessing here.  But the problem is that
>   when can do info migrate after ending a migration.  And at that time
>   qemu-file is gone, so we add another counter instead of storing the
>   value of qemu-file.
> 
> Should I continue, and search for the patches that changed the things,
> or can we agree that this is a complex problem and can't be fixed with
> yet another one line?

I don't think this is the same issue we're discussing.. but I understand
we may need a lot of changes to fix all the counters.

My request was much simpler, IMHO:

It's about making sure every patch is self contained, and that's all of it.
Each patch needs to justify itself, if the code change can't, the commit
message should supplement that and make sure anyone stumbles over the patch
can have a way to trace back on why it happened.  That's all I was asking.

Now when I goes back and look at this simplest patch, again I'd just squash
this back to previous as I used to comment.  If not, I think the only
missing piece / last question is, why you didn't remove the last
total_transferred in the last patch, where anyway you removed all the rest:

https://lore.kernel.org/r/20230530183941.7223-6-quintela@redhat.com

===8<===
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index be3dab85cb..eb0497e532 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -288,6 +288,7 @@ void qemu_fflush(QEMUFile *f)
         } else {
             uint64_t size = iov_size(f->iov, f->iovcnt);
             f->total_transferred += size;        <------------------- this is NOT removed
+            stat64_add(&mig_stats.qemu_file_transferred, size);
         }

         qemu_iovec_release_ram(f);
@@ -628,7 +629,7 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)

 uint64_t qemu_file_transferred_noflush(QEMUFile *f)
 {
-    uint64_t ret = f->total_transferred;         <------------------- this is removed
+    uint64_t ret = stat64_get(&mig_stats.qemu_file_transferred);
     int i;

     g_assert(qemu_file_is_writable(f));
@@ -644,7 +645,7 @@ uint64_t qemu_file_transferred(QEMUFile *f)
 {
     g_assert(qemu_file_is_writable(f));
     qemu_fflush(f);
-    return f->total_transferred;                 <------------------- this is removed
+    return stat64_get(&mig_stats.qemu_file_transferred);
 }
===8<===

I think that's why I said "this patch proved something was wrong before",
and it's as simple as that..

As a reviewer, I hope we, as qemu migration submodule, follows the same bar
on review with major linux communities.  This patch is probably not gonna
be accepted in any of known sub-communities I'm familiar with.

So if you want to keep removal of total_transferred standalone, please move
the other line to previous patch from this patch.  But again, I really
don't see why it's needed at all to split a separate patch just to remove
total_transferred.

> I spent the best part of a couple of months
> trying to fix the problem with one liners, and ended without fixing the
> problem after too many one liners.
> 
> Ended spending another couple of months writting changing the code
> correctly, simplifying the number of counters and giving the same
> functionality that was before.  But it took too many patches.
> 
> And why it ends with so many patches?
> I am glad that you asked.  Because I find a bug.  And I try to fix it.
> And then I see that there is another thing that I need to fix to be able
> to fix this one.  And another.  And another.
> 
> > What I have in mind are:
> >
> >   - The regression you mentioned on qemu_fflush() when ram save (I hope I
> >     understand the problem right, though...).
> 
> After the PULL request that I am about to send, we need to get another 4
> patches reviewed.

Could you help list them out?  I can review them today and get it fixed
asap.

> 
> >   - The slowness of migration-test.  I'm not sure whether you have anything
> >     better than either Dan's approach
> 
> too complex for my taste, and don't get all the speed back.
> 
> > or the switchover-hold flags,
> 
> I proposed this, but in a different way, will try to send something
> before the week ends, sorry for the delay.

That's not a major problem yet to me, as long as others are fine with
migration-test being slow.  Glad to read and review it.

Thanks,

-- 
Peter Xu