[PATCH 0/9] QEMU file cleanups

[PATCH 0/9] QEMU file cleanups

[Juan Quintela]

Hi

While I am trying to put order in the atomic counters, I made in this series:

- convince and review code to see that everything is uint64_t.

- f->shutdown is not needed.  When we shutdown the file we put an
  error there if there is none.  So remove it.
- Make more clear how we use rate_limit.

Please review.

It is based on my previous series to the list:
Subject: [PATCH 0/2] More migration stats
Based-on: Message-Id: <20230504103357.22130-1-quintela@redhat.com>

Juan Quintela (9):
  migration: max_postcopy_bandwidth is a size parameter
  migration: qemu_file_total_transferred() function is monotonic
  qemu-file: make qemu_file_[sg]et_rate_limit() use an uint64_t
  qemu-file: Make rate_limit_used an uint64_t
  qemu-file: No need to check for shutdown in qemu_file_rate_limit
  qemu-file: remove shutdown member
  qemu-file: Make total_transferred an uint64_t
  qemu-file: Make ram_control_save_page() use accessors for rate_limit
  qemu-file: Account for rate_limit usage on qemu_fflush()

 migration/block.c     | 13 +++----------
 migration/migration.c | 10 +++++-----
 migration/options.c   |  2 +-
 migration/options.h   |  2 +-
 migration/qemu-file.c | 42 +++++++++++++++++-------------------------
 migration/qemu-file.h | 10 +++++-----
 migration/savevm.c    |  6 ++----
 migration/vmstate.c   |  2 +-
 8 files changed, 35 insertions(+), 52 deletions(-)

-- 
2.40.0

[PATCH 1/9] migration: max_postcopy_bandwidth is a size parameter

[Juan Quintela]

So make everything that uses it uint64_t no int64_t.

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

diff --git a/migration/migration.c b/migration/migration.c
index feb5ab7493..232e387109 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2056,7 +2056,7 @@ static int postcopy_start(MigrationState *ms)
     QIOChannelBuffer *bioc;
     QEMUFile *fb;
     int64_t time_at_stop = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-    int64_t bandwidth = migrate_max_postcopy_bandwidth();
+    uint64_t bandwidth = migrate_max_postcopy_bandwidth();
     bool restart_block = false;
     int cur_state = MIGRATION_STATUS_ACTIVE;
 
@@ -3176,7 +3176,7 @@ fail:
 void migrate_fd_connect(MigrationState *s, Error *error_in)
 {
     Error *local_err = NULL;
-    int64_t rate_limit;
+    uint64_t rate_limit;
     bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
 
     /*
diff --git a/migration/options.c b/migration/options.c
index 7395787960..2e759cc306 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -717,7 +717,7 @@ uint64_t migrate_max_bandwidth(void)
     return s->parameters.max_bandwidth;
 }
 
-int64_t migrate_max_postcopy_bandwidth(void)
+uint64_t migrate_max_postcopy_bandwidth(void)
 {
     MigrationState *s = migrate_get_current();
 
diff --git a/migration/options.h b/migration/options.h
index 09841d6a63..5cca3326d6 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -85,7 +85,7 @@ int migrate_decompress_threads(void);
 uint64_t migrate_downtime_limit(void);
 uint8_t migrate_max_cpu_throttle(void);
 uint64_t migrate_max_bandwidth(void);
-int64_t migrate_max_postcopy_bandwidth(void);
+uint64_t migrate_max_postcopy_bandwidth(void);
 int migrate_multifd_channels(void);
 MultiFDCompression migrate_multifd_compression(void);
 int migrate_multifd_zlib_level(void);
-- 
2.40.0

[PATCH 2/9] migration: qemu_file_total_transferred() function is monotonic

[Juan Quintela]

So delta_bytes can only be greater or equal to zero.  Never negative.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/block.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index 6d532ac7a2..3499f75e37 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -801,13 +801,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
 
     qemu_put_be64(f, BLK_MIG_FLAG_EOS);
     delta_bytes = qemu_file_total_transferred(f) - last_bytes;
-    if (delta_bytes > 0) {
-        return 1;
-    } else if (delta_bytes < 0) {
-        return -1;
-    } else {
-        return 0;
-    }
+    return (delta_bytes > 0);
 }
 
 /* Called with iothread lock taken.  */
-- 
2.40.0

[PATCH 3/9] qemu-file: make qemu_file_[sg]et_rate_limit() use an uint64_t

[Juan Quintela]

It is really size_t.  Everything else uses uint64_t, so move this to
uint64_t as well.  A size can't be negative anyways.

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

diff --git a/migration/migration.c b/migration/migration.c
index 232e387109..ee75c6cfbd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2119,7 +2119,7 @@ static int postcopy_start(MigrationState *ms)
      */
     /* 0 max-postcopy-bandwidth means unlimited */
     if (!bandwidth) {
-        qemu_file_set_rate_limit(ms->to_dst_file, INT64_MAX);
+        qemu_file_set_rate_limit(ms->to_dst_file, UINT64_MAX);
     } else {
         qemu_file_set_rate_limit(ms->to_dst_file, bandwidth / XFER_LIMIT_RATIO);
     }
@@ -2301,7 +2301,7 @@ static void migration_completion(MigrationState *s)
             }
             if (ret >= 0) {
                 s->block_inactive = !migrate_colo();
-                qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
+                qemu_file_set_rate_limit(s->to_dst_file, UINT64_MAX);
                 ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
                                                          s->block_inactive);
             }
@@ -3049,7 +3049,7 @@ static void *bg_migration_thread(void *opaque)
     rcu_register_thread();
     object_ref(OBJECT(s));
 
-    qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
+    qemu_file_set_rate_limit(s->to_dst_file, UINT64_MAX);
 
     setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
     /*
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index ee04240a21..b322b363cf 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -43,7 +43,7 @@ struct QEMUFile {
      * Maximum amount of data in bytes to transfer during one
      * rate limiting time window
      */
-    int64_t rate_limit_max;
+    uint64_t rate_limit_max;
     /*
      * Total amount of data in bytes queued for transfer
      * during this rate limiting time window
@@ -748,18 +748,18 @@ int qemu_file_rate_limit(QEMUFile *f)
     if (qemu_file_get_error(f)) {
         return 1;
     }
-    if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) {
+    if (f->rate_limit_used > f->rate_limit_max) {
         return 1;
     }
     return 0;
 }
 
-int64_t qemu_file_get_rate_limit(QEMUFile *f)
+uint64_t qemu_file_get_rate_limit(QEMUFile *f)
 {
     return f->rate_limit_max;
 }
 
-void qemu_file_set_rate_limit(QEMUFile *f, int64_t limit)
+void qemu_file_set_rate_limit(QEMUFile *f, uint64_t limit)
 {
     f->rate_limit_max = limit;
 }
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index d16cd50448..9e158c00f6 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -138,8 +138,8 @@ void qemu_file_reset_rate_limit(QEMUFile *f);
  * need to be applied to the rate limiting calcuations
  */
 void qemu_file_acct_rate_limit(QEMUFile *f, int64_t len);
-void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
-int64_t qemu_file_get_rate_limit(QEMUFile *f);
+void qemu_file_set_rate_limit(QEMUFile *f, uint64_t new_rate);
+uint64_t qemu_file_get_rate_limit(QEMUFile *f);
 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);
-- 
2.40.0

[PATCH 4/9] qemu-file: Make rate_limit_used an uint64_t

[Juan Quintela]

Change all the functions that use it.  It was already passed as
uint64_t.

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

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index b322b363cf..5aa990b82a 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -48,7 +48,7 @@ struct QEMUFile {
      * Total amount of data in bytes queued for transfer
      * during this rate limiting time window
      */
-    int64_t rate_limit_used;
+    uint64_t rate_limit_used;
 
     /* The sum of bytes transferred on the wire */
     int64_t total_transferred;
@@ -769,7 +769,7 @@ void qemu_file_reset_rate_limit(QEMUFile *f)
     f->rate_limit_used = 0;
 }
 
-void qemu_file_acct_rate_limit(QEMUFile *f, int64_t len)
+void qemu_file_acct_rate_limit(QEMUFile *f, uint64_t len)
 {
     f->rate_limit_used += len;
 }
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 9e158c00f6..55ef5a2ac7 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -137,7 +137,7 @@ void qemu_file_reset_rate_limit(QEMUFile *f);
  * out of band from the main file object I/O methods, and
  * need to be applied to the rate limiting calcuations
  */
-void qemu_file_acct_rate_limit(QEMUFile *f, int64_t len);
+void qemu_file_acct_rate_limit(QEMUFile *f, uint64_t len);
 void qemu_file_set_rate_limit(QEMUFile *f, uint64_t new_rate);
 uint64_t qemu_file_get_rate_limit(QEMUFile *f);
 int qemu_file_get_error_obj(QEMUFile *f, Error **errp);
-- 
2.40.0

[PATCH 5/9] qemu-file: No need to check for shutdown in qemu_file_rate_limit

[Juan Quintela]

After calling qemu_file_shutdown() we set the error as -EIO if there
is no another previous error, so no need to check it here.

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

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 5aa990b82a..771aba7369 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -742,9 +742,6 @@ int64_t qemu_file_total_transferred(QEMUFile *f)
 
 int qemu_file_rate_limit(QEMUFile *f)
 {
-    if (f->shutdown) {
-        return 1;
-    }
     if (qemu_file_get_error(f)) {
         return 1;
     }
-- 
2.40.0

[PATCH 6/9] qemu-file: remove shutdown member

[Juan Quintela]

The first thing that we do after setting the shutdown value is set the
error as -EIO if there is not a previous error.

So this value is reduntant.  Just remove it and use
qemu_file_get_error() in the places that it was tested.

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

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 771aba7369..7b32ef45a9 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -63,8 +63,6 @@ struct QEMUFile {
 
     int last_error;
     Error *last_error_obj;
-    /* has the file has been shutdown */
-    bool shutdown;
 };
 
 /*
@@ -78,8 +76,6 @@ int qemu_file_shutdown(QEMUFile *f)
 {
     int ret = 0;
 
-    f->shutdown = true;
-
     /*
      * We must set qemufile error before the real shutdown(), otherwise
      * there can be a race window where we thought IO all went though
@@ -294,7 +290,7 @@ void qemu_fflush(QEMUFile *f)
         return;
     }
 
-    if (f->shutdown) {
+    if (qemu_file_get_error(f)) {
         return;
     }
     if (f->iovcnt > 0) {
@@ -407,7 +403,7 @@ static ssize_t coroutine_mixed_fn qemu_fill_buffer(QEMUFile *f)
     f->buf_index = 0;
     f->buf_size = pending;
 
-    if (f->shutdown) {
+    if (qemu_file_get_error(f)) {
         return 0;
     }
 
@@ -496,7 +492,7 @@ static int add_to_iovec(QEMUFile *f, const uint8_t *buf, size_t size,
     } else {
         if (f->iovcnt >= MAX_IOV_SIZE) {
             /* Should only happen if a previous fflush failed */
-            assert(f->shutdown || !qemu_file_is_writable(f));
+            assert(qemu_file_get_error(f) || !qemu_file_is_writable(f));
             return 1;
         }
         if (may_free) {
-- 
2.40.0

[PATCH 7/9] qemu-file: Make total_transferred an uint64_t

[Juan Quintela]

Change all the functions that use it.  It was already passed as
uint64_t.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/block.c     | 5 ++---
 migration/qemu-file.c | 8 ++++----
 migration/qemu-file.h | 4 ++--
 migration/savevm.c    | 6 ++----
 migration/vmstate.c   | 2 +-
 5 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index 3499f75e37..a37678ce95 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -747,8 +747,7 @@ static int block_save_setup(QEMUFile *f, void *opaque)
 static int block_save_iterate(QEMUFile *f, void *opaque)
 {
     int ret;
-    int64_t last_bytes = qemu_file_total_transferred(f);
-    int64_t delta_bytes;
+    uint64_t last_bytes = qemu_file_total_transferred(f);
 
     trace_migration_block_save("iterate", block_mig_state.submitted,
                                block_mig_state.transferred);
@@ -800,7 +799,7 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
     }
 
     qemu_put_be64(f, BLK_MIG_FLAG_EOS);
-    delta_bytes = qemu_file_total_transferred(f) - last_bytes;
+    uint64_t delta_bytes = qemu_file_total_transferred(f) - last_bytes;
     return (delta_bytes > 0);
 }
 
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 7b32ef45a9..e97d180f17 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -51,7 +51,7 @@ struct QEMUFile {
     uint64_t rate_limit_used;
 
     /* The sum of bytes transferred on the wire */
-    int64_t total_transferred;
+    uint64_t total_transferred;
 
     int buf_index;
     int buf_size; /* 0 when writing */
@@ -718,9 +718,9 @@ int coroutine_mixed_fn qemu_get_byte(QEMUFile *f)
     return result;
 }
 
-int64_t qemu_file_total_transferred_fast(QEMUFile *f)
+uint64_t qemu_file_total_transferred_fast(QEMUFile *f)
 {
-    int64_t ret = f->total_transferred;
+    uint64_t ret = f->total_transferred;
     int i;
 
     for (i = 0; i < f->iovcnt; i++) {
@@ -730,7 +730,7 @@ int64_t qemu_file_total_transferred_fast(QEMUFile *f)
     return ret;
 }
 
-int64_t qemu_file_total_transferred(QEMUFile *f)
+uint64_t qemu_file_total_transferred(QEMUFile *f)
 {
     qemu_fflush(f);
     return f->total_transferred;
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 55ef5a2ac7..d758e7f10b 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -83,7 +83,7 @@ int qemu_fclose(QEMUFile *f);
  *
  * Returns: the total bytes transferred
  */
-int64_t qemu_file_total_transferred(QEMUFile *f);
+uint64_t qemu_file_total_transferred(QEMUFile *f);
 
 /*
  * qemu_file_total_transferred_fast:
@@ -95,7 +95,7 @@ int64_t qemu_file_total_transferred(QEMUFile *f);
  *
  * Returns: the total bytes transferred and queued
  */
-int64_t qemu_file_total_transferred_fast(QEMUFile *f);
+uint64_t qemu_file_total_transferred_fast(QEMUFile *f);
 
 /*
  * put_buffer without copying the buffer.
diff --git a/migration/savevm.c b/migration/savevm.c
index a9d0a88e62..032044b1d5 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -927,11 +927,9 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
 static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
                                    JSONWriter *vmdesc)
 {
-    int64_t old_offset, size;
-
-    old_offset = qemu_file_total_transferred_fast(f);
+    uint64_t old_offset = qemu_file_total_transferred_fast(f);
     se->ops->save_state(f, se->opaque);
-    size = qemu_file_total_transferred_fast(f) - old_offset;
+    uint64_t size = qemu_file_total_transferred_fast(f) - old_offset;
 
     if (vmdesc) {
         json_writer_int64(vmdesc, "size", size);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 83ca4c7d3e..351f56104e 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -349,7 +349,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
             void *first_elem = opaque + field->offset;
             int i, n_elems = vmstate_n_elems(opaque, field);
             int size = vmstate_size(opaque, field);
-            int64_t old_offset, written_bytes;
+            uint64_t old_offset, written_bytes;
             JSONWriter *vmdesc_loop = vmdesc;
 
             trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
-- 
2.40.0

[PATCH 8/9] qemu-file: Make ram_control_save_page() use accessors for rate_limit

[Juan Quintela]

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 e97d180f17..ddebfac847 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -362,7 +362,7 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
         int ret = f->hooks->save_page(f, block_offset,
                                       offset, size, bytes_sent);
         if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
-            f->rate_limit_used += size;
+            qemu_file_acct_rate_limit(f, size);
         }
 
         if (ret != RAM_SAVE_CONTROL_DELAYED &&
-- 
2.40.0

[PATCH 9/9] qemu-file: Account for rate_limit usage on qemu_fflush()

[Juan Quintela]

That is the moment we know we have transferred something.

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

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index ddebfac847..309b4c56f4 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -300,7 +300,9 @@ void qemu_fflush(QEMUFile *f)
                                    &local_error) < 0) {
             qemu_file_set_error_obj(f, -EIO, local_error);
         } else {
-            f->total_transferred += iov_size(f->iov, f->iovcnt);
+            uint64_t size = iov_size(f->iov, f->iovcnt);
+            qemu_file_acct_rate_limit(f, size);
+            f->total_transferred += size;
         }
 
         qemu_iovec_release_ram(f);
@@ -527,7 +529,6 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
         return;
     }
 
-    f->rate_limit_used += size;
     add_to_iovec(f, buf, size, may_free);
 }
 
@@ -545,7 +546,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size)
             l = size;
         }
         memcpy(f->buf + f->buf_index, buf, l);
-        f->rate_limit_used += l;
         add_buf_to_iovec(f, l);
         if (qemu_file_get_error(f)) {
             break;
@@ -562,7 +562,6 @@ void qemu_put_byte(QEMUFile *f, int v)
     }
 
     f->buf[f->buf_index] = v;
-    f->rate_limit_used++;
     add_buf_to_iovec(f, 1);
 }
 
-- 
2.40.0

Re: [PATCH 5/9] qemu-file: No need to check for shutdown in qemu_file_rate_limit

[Peter Xu]

On Thu, May 04, 2023 at 01:38:37PM +0200, Juan Quintela wrote:
> After calling qemu_file_shutdown() we set the error as -EIO if there
> is no another previous error, so no need to check it here.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

-- 
Peter Xu

Re: [PATCH 6/9] qemu-file: remove shutdown member

[Peter Xu]

On Thu, May 04, 2023 at 01:38:38PM +0200, Juan Quintela wrote:
> The first thing that we do after setting the shutdown value is set the
> error as -EIO if there is not a previous error.
> 
> So this value is reduntant.  Just remove it and use
> qemu_file_get_error() in the places that it was tested.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Nit: I'd squash this with previous.

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

-- 
Peter Xu

Re: [PATCH 8/9] qemu-file: Make ram_control_save_page() use accessors for rate_limit

[Peter Xu]

On Thu, May 04, 2023 at 01:38:40PM +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>

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

-- 
Peter Xu

Re: [PATCH 9/9] qemu-file: Account for rate_limit usage on qemu_fflush()

[Peter Xu]

On Thu, May 04, 2023 at 01:38:41PM +0200, Juan Quintela wrote:
> That is the moment we know we have transferred something.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

There'll be a slight side effect that qemu_file_rate_limit() can be
triggered later than before because data cached in the qemufile won't be
accounted until flushed.

Two limits here:

- IO_BUF_SIZE==32K, the real buffer
- MAX_IOV_SIZE==64 (I think), the async buffer to put guest page ptrs
  directly, on x86 it's 64*4K=256K

So the impact should be no more than 288KB on x86.  Looks still fine..

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

-- 
Peter Xu

Re: [PATCH 0/9] QEMU file cleanups

[Peter Xu]

On Thu, May 04, 2023 at 01:38:32PM +0200, Juan Quintela wrote:
> - convince and review code to see that everything is uint64_t.

One general question to patches regarding this - what's the major benefit
of using uint64_t?

It doubles the possible numbers to hold, but it's already 64bits so I don't
think it matters a lot.  The thing is we're removing some code trying to
detect negative which seems to be still helpful to detect e.g. overflows
(even though I don't think it'll happen).  I just still think it's good to
know when overflow happens, and not sure what I missed on benefits of using
unsigned here.

I've reviewed all the rest patches and all look good here.

Thanks,

-- 
Peter Xu

Re: [PATCH 0/9] QEMU file cleanups

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Thu, May 04, 2023 at 01:38:32PM +0200, Juan Quintela wrote:
>> - convince and review code to see that everything is uint64_t.
>
> One general question to patches regarding this - what's the major benefit
> of using uint64_t?
>
> It doubles the possible numbers to hold, but it's already 64bits so I don't
> think it matters a lot.

We were checking for negatives even when that can't be.
And we are doing this dance of

int64_t x, y;
uint64_t a, b;

x = a;
b = y;

This is always confusing and not always right.

> The thing is we're removing some code trying to
> detect negative which seems to be still helpful to detect e.g. overflows
> (even though I don't think it'll happen).  I just still think it's good to
> know when overflow happens, and not sure what I missed on benefits of using
> unsigned here.

If you grep through the code, you see that half of the things are
int64_t and the other half is uint64_t.  I find it always confusing.


> I've reviewed all the rest patches and all look good here.

Thanks very much.

Re: [PATCH 9/9] qemu-file: Account for rate_limit usage on qemu_fflush()

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Thu, May 04, 2023 at 01:38:41PM +0200, Juan Quintela wrote:
>> That is the moment we know we have transferred something.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> There'll be a slight side effect that qemu_file_rate_limit() can be
> triggered later than before because data cached in the qemufile won't be
> accounted until flushed.
>
> Two limits here:
>
> - IO_BUF_SIZE==32K, the real buffer
> - MAX_IOV_SIZE==64 (I think), the async buffer to put guest page ptrs
>   directly, on x86 it's 64*4K=256K
>
> So the impact should be no more than 288KB on x86.  Looks still fine..

This was on purpose.  We are counting data as sent when it has not been
sent yet.

With this change, we account for the data written when we do the real
write.

And yes, I fully agree that with the size that we have it shouldn't make
much of a difference in the speed calculation.

The difference here is that I will move that counter somewhere else, and
the less places that I have to use it the better O:-)

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

Thanks.

Re: [PATCH 0/9] QEMU file cleanups

[Peter Xu]

On Thu, May 04, 2023 at 04:56:46PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Thu, May 04, 2023 at 01:38:32PM +0200, Juan Quintela wrote:
> >> - convince and review code to see that everything is uint64_t.
> >
> > One general question to patches regarding this - what's the major benefit
> > of using uint64_t?
> >
> > It doubles the possible numbers to hold, but it's already 64bits so I don't
> > think it matters a lot.
> 
> We were checking for negatives even when that can't be.
> And we are doing this dance of
> 
> int64_t x, y;
> uint64_t a, b;
> 
> x = a;
> b = y;
> 
> This is always confusing and not always right.

Yeah this is confusing, but if anything can go wrong with this I assume we
could have some bigger problem anyway..

> 
> > The thing is we're removing some code trying to
> > detect negative which seems to be still helpful to detect e.g. overflows
> > (even though I don't think it'll happen).  I just still think it's good to
> > know when overflow happens, and not sure what I missed on benefits of using
> > unsigned here.
> 
> If you grep through the code, you see that half of the things are
> int64_t and the other half is uint64_t.  I find it always confusing.

Right, I'm personally curious whether we should just use int64_t always
unless necessary. :) Another good thing with int64_t is it's also suitable
for error report when used in retvals.

But no strong opinion here, I don't think that's a huge deal for now.
Having such an alignment on types makes sense to me.

Thanks,

-- 
Peter Xu

Re: [PATCH 0/9] QEMU file cleanups

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Thu, May 04, 2023 at 04:56:46PM +0200, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > On Thu, May 04, 2023 at 01:38:32PM +0200, Juan Quintela wrote:
>> >> - convince and review code to see that everything is uint64_t.
>> >
>> > One general question to patches regarding this - what's the major benefit
>> > of using uint64_t?
>> >
>> > It doubles the possible numbers to hold, but it's already 64bits so I don't
>> > think it matters a lot.
>> 
>> We were checking for negatives even when that can't be.
>> And we are doing this dance of
>> 
>> int64_t x, y;
>> uint64_t a, b;
>> 
>> x = a;
>> b = y;
>> 
>> This is always confusing and not always right.
>
> Yeah this is confusing, but if anything can go wrong with this I assume we
> could have some bigger problem anyway..
>
>> 
>> > The thing is we're removing some code trying to
>> > detect negative which seems to be still helpful to detect e.g. overflows
>> > (even though I don't think it'll happen).  I just still think it's good to
>> > know when overflow happens, and not sure what I missed on benefits of using
>> > unsigned here.
>> 
>> If you grep through the code, you see that half of the things are
>> int64_t and the other half is uint64_t.  I find it always confusing.
>
> Right, I'm personally curious whether we should just use int64_t always
> unless necessary. :) Another good thing with int64_t is it's also suitable
> for error report when used in retvals.

It is used by sizes.  We don't return errors right now.
And if it is negative, we need to check that if it is negative, even
when no code uses the negative functionality.

> But no strong opinion here, I don't think that's a huge deal for now.
> Having such an alignment on types makes sense to me.

Thanks, Juan.

Re: [PATCH 1/9] migration: max_postcopy_bandwidth is a size parameter

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:38:33PM +0200, Juan Quintela wrote:
> So make everything that uses it uint64_t no int64_t.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 4 ++--
>  migration/options.c   | 2 +-
>  migration/options.h   | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/9] migration: qemu_file_total_transferred() function is monotonic

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:38:34PM +0200, Juan Quintela wrote:
> So delta_bytes can only be greater or equal to zero.  Never negative.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/block.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/9] qemu-file: make qemu_file_[sg]et_rate_limit() use an uint64_t

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:38:35PM +0200, Juan Quintela wrote:
> It is really size_t.  Everything else uses uint64_t, so move this to
> uint64_t as well.  A size can't be negative anyways.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 6 +++---
>  migration/qemu-file.c | 8 ++++----
>  migration/qemu-file.h | 4 ++--
>  3 files changed, 9 insertions(+), 9 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 4/9] qemu-file: Make rate_limit_used an uint64_t

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:38:36PM +0200, Juan Quintela wrote:
> Change all the functions that use it.  It was already passed as
> uint64_t.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.c | 4 ++--
>  migration/qemu-file.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 5/9] qemu-file: No need to check for shutdown in qemu_file_rate_limit

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:38:37PM +0200, Juan Quintela wrote:
> After calling qemu_file_shutdown() we set the error as -EIO if there
> is no another previous error, so no need to check it here.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.c | 3 ---
>  1 file changed, 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 6/9] qemu-file: remove shutdown member

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:38:38PM +0200, Juan Quintela wrote:
> The first thing that we do after setting the shutdown value is set the
> error as -EIO if there is not a previous error.
> 
> So this value is reduntant.  Just remove it and use

  s/reduntant/redundant/

> qemu_file_get_error() in the places that it was tested.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 7/9] qemu-file: Make total_transferred an uint64_t

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:38:39PM +0200, Juan Quintela wrote:
> Change all the functions that use it.  It was already passed as
> uint64_t.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/block.c     | 5 ++---
>  migration/qemu-file.c | 8 ++++----
>  migration/qemu-file.h | 4 ++--
>  migration/savevm.c    | 6 ++----
>  migration/vmstate.c   | 2 +-
>  5 files changed, 11 insertions(+), 14 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 8/9] qemu-file: Make ram_control_save_page() use accessors for rate_limit

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:38:40PM +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 9/9] qemu-file: Account for rate_limit usage on qemu_fflush()

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 01:38:41PM +0200, Juan Quintela wrote:
> That is the moment we know we have transferred something.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index ddebfac847..309b4c56f4 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -300,7 +300,9 @@ void qemu_fflush(QEMUFile *f)
>                                     &local_error) < 0) {
>              qemu_file_set_error_obj(f, -EIO, local_error);
>          } else {
> -            f->total_transferred += iov_size(f->iov, f->iovcnt);
> +            uint64_t size = iov_size(f->iov, f->iovcnt);
> +            qemu_file_acct_rate_limit(f, size);
> +            f->total_transferred += size;
>          }
>  
>          qemu_iovec_release_ram(f);
> @@ -527,7 +529,6 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
>          return;
>      }
>  
> -    f->rate_limit_used += size;
>      add_to_iovec(f, buf, size, may_free);
>  }
>  
> @@ -545,7 +546,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size)
>              l = size;
>          }
>          memcpy(f->buf + f->buf_index, buf, l);
> -        f->rate_limit_used += l;
>          add_buf_to_iovec(f, l);
>          if (qemu_file_get_error(f)) {
>              break;
> @@ -562,7 +562,6 @@ void qemu_put_byte(QEMUFile *f, int v)
>      }
>  
>      f->buf[f->buf_index] = v;
> -    f->rate_limit_used++;
>      add_buf_to_iovec(f, 1);
>  }

This has a slight semantic behavioural change.

By accounting for rate limit in the qemu_put functions, we ensure
that we stop growing the iovec when rate limiting activates.

If we only apply rate limit in the the flush function, that will
let the  f->iov continue to accumulate buffers, while we have
rate limited the actual transfer. This makes me uneasy - it feels
like a bad idea to continue to accumulate buffers if we're not
ready to send them

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 9/9] qemu-file: Account for rate_limit usage on qemu_fflush()

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, May 04, 2023 at 01:38:41PM +0200, Juan Quintela wrote:
>> That is the moment we know we have transferred something.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/qemu-file.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
>> index ddebfac847..309b4c56f4 100644
>> --- a/migration/qemu-file.c
>> +++ b/migration/qemu-file.c
>> @@ -300,7 +300,9 @@ void qemu_fflush(QEMUFile *f)
>>                                     &local_error) < 0) {
>>              qemu_file_set_error_obj(f, -EIO, local_error);
>>          } else {
>> -            f->total_transferred += iov_size(f->iov, f->iovcnt);
>> +            uint64_t size = iov_size(f->iov, f->iovcnt);
>> +            qemu_file_acct_rate_limit(f, size);
>> +            f->total_transferred += size;
>>          }
>>  
>>          qemu_iovec_release_ram(f);
>> @@ -527,7 +529,6 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
>>          return;
>>      }
>>  
>> -    f->rate_limit_used += size;
>>      add_to_iovec(f, buf, size, may_free);
>>  }
>>  
>> @@ -545,7 +546,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size)
>>              l = size;
>>          }
>>          memcpy(f->buf + f->buf_index, buf, l);
>> -        f->rate_limit_used += l;
>>          add_buf_to_iovec(f, l);
>>          if (qemu_file_get_error(f)) {
>>              break;
>> @@ -562,7 +562,6 @@ void qemu_put_byte(QEMUFile *f, int v)
>>      }
>>  
>>      f->buf[f->buf_index] = v;
>> -    f->rate_limit_used++;
>>      add_buf_to_iovec(f, 1);
>>  }
>
> This has a slight semantic behavioural change.

Yeap.

See the answer to Peter.  But three things came to mind:

a - the size of the buffer is small (between 32KB and 256KB depending
    how you count it).  So we are going to call qemu_fflush() really
    soon.

b - We are using this value to calculate how much we can send through
    the wire.  Here we are saything how much we have accepted to send.

c - When using multifd the number of bytes that we send through the qemu
    file is even smaller. migration-test multifd test send 300MB of data
    through multifd channels and around 300KB on the qemu_file channel.


>
> By accounting for rate limit in the qemu_put functions, we ensure
> that we stop growing the iovec when rate limiting activates.
>
> If we only apply rate limit in the the flush function, that will
> let the  f->iov continue to accumulate buffers, while we have
> rate limited the actual transfer.

256KB maximum.  Our accounting has bigger errors than that.


> This makes me uneasy - it feels like a bad idea to continue to
> accumulate buffers if we're not ready to send them

I still think that the change is correct.  But as you and Peter have
concerns about it, I will think a bit more about it.

Thanks, Juan.

Re: [PATCH 3/9] qemu-file: make qemu_file_[sg]et_rate_limit() use an uint64_t

[Juan Quintela]

Juan Quintela <quintela@redhat.com> wrote:
> It is really size_t.  Everything else uses uint64_t, so move this to
> uint64_t as well.  A size can't be negative anyways.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Self-nack.

> -        qemu_file_set_rate_limit(ms->to_dst_file, INT64_MAX);
> +        qemu_file_set_rate_limit(ms->to_dst_file, UINT64_MAX);

1st: this should be zero.

>      } else {
>          qemu_file_set_rate_limit(ms->to_dst_file, bandwidth / XFER_LIMIT_RATIO);
>      }
> @@ -2301,7 +2301,7 @@ static void migration_completion(MigrationState *s)
>              }
>              if (ret >= 0) {
>                  s->block_inactive = !migrate_colo();
> -                qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
> +                qemu_file_set_rate_limit(s->to_dst_file, UINT64_MAX);

Same here

>                  ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>                                                           s->block_inactive);
>              }


> @@ -3049,7 +3049,7 @@ static void *bg_migration_thread(void *opaque)
>      rcu_register_thread();
>      object_ref(OBJECT(s));
>  
> -    qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
> +    qemu_file_set_rate_limit(s->to_dst_file, UINT64_MAX);

And here.

> @@ -748,18 +748,18 @@ int qemu_file_rate_limit(QEMUFile *f)
>      if (qemu_file_get_error(f)) {
>          return 1;
>      }
> -    if (f->rate_limit_max > 0 && f->rate_limit_used > f->rate_limit_max) {
> +    if (f->rate_limit_used > f->rate_limit_max) {

And this is wrong.  f->rate_limit_max == 0 means that we don't do  rate_limit.

Will resend this one later.

Sorry, Juan.

PD.  No, I have no clue how I have had this patch applied the whole day
     and no failures and now I get failures in migration-test.  The
     number of times that I run this test on the last two days have been
     in the hundreds.

Re: [PATCH 9/9] qemu-file: Account for rate_limit usage on qemu_fflush()

[Daniel P. Berrangé]

On Thu, May 04, 2023 at 07:22:25PM +0200, Juan Quintela wrote:
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> > On Thu, May 04, 2023 at 01:38:41PM +0200, Juan Quintela wrote:
> >> That is the moment we know we have transferred something.
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  migration/qemu-file.c | 7 +++----
> >>  1 file changed, 3 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> >> index ddebfac847..309b4c56f4 100644
> >> --- a/migration/qemu-file.c
> >> +++ b/migration/qemu-file.c
> >> @@ -300,7 +300,9 @@ void qemu_fflush(QEMUFile *f)
> >>                                     &local_error) < 0) {
> >>              qemu_file_set_error_obj(f, -EIO, local_error);
> >>          } else {
> >> -            f->total_transferred += iov_size(f->iov, f->iovcnt);
> >> +            uint64_t size = iov_size(f->iov, f->iovcnt);
> >> +            qemu_file_acct_rate_limit(f, size);
> >> +            f->total_transferred += size;
> >>          }
> >>  
> >>          qemu_iovec_release_ram(f);
> >> @@ -527,7 +529,6 @@ void qemu_put_buffer_async(QEMUFile *f, const uint8_t *buf, size_t size,
> >>          return;
> >>      }
> >>  
> >> -    f->rate_limit_used += size;
> >>      add_to_iovec(f, buf, size, may_free);
> >>  }
> >>  
> >> @@ -545,7 +546,6 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, size_t size)
> >>              l = size;
> >>          }
> >>          memcpy(f->buf + f->buf_index, buf, l);
> >> -        f->rate_limit_used += l;
> >>          add_buf_to_iovec(f, l);
> >>          if (qemu_file_get_error(f)) {
> >>              break;
> >> @@ -562,7 +562,6 @@ void qemu_put_byte(QEMUFile *f, int v)
> >>      }
> >>  
> >>      f->buf[f->buf_index] = v;
> >> -    f->rate_limit_used++;
> >>      add_buf_to_iovec(f, 1);
> >>  }
> >
> > This has a slight semantic behavioural change.
> 
> Yeap.
> 
> See the answer to Peter.  But three things came to mind:
> 
> a - the size of the buffer is small (between 32KB and 256KB depending
>     how you count it).  So we are going to call qemu_fflush() really
>     soon.
> 
> b - We are using this value to calculate how much we can send through
>     the wire.  Here we are saything how much we have accepted to send.
> 
> c - When using multifd the number of bytes that we send through the qemu
>     file is even smaller. migration-test multifd test send 300MB of data
>     through multifd channels and around 300KB on the qemu_file channel.
> 
> 
> >
> > By accounting for rate limit in the qemu_put functions, we ensure
> > that we stop growing the iovec when rate limiting activates.
> >
> > If we only apply rate limit in the the flush function, that will
> > let the  f->iov continue to accumulate buffers, while we have
> > rate limited the actual transfer.
> 
> 256KB maximum.  Our accounting has bigger errors than that.
> 
> 
> > This makes me uneasy - it feels like a bad idea to continue to
> > accumulate buffers if we're not ready to send them
> 
> I still think that the change is correct.  But as you and Peter have
> concerns about it, I will think a bit more about it.

If Peter's calculations are correct, then I don't have any objection,
as that's a small overhead.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 9/9] qemu-file: Account for rate_limit usage on qemu_fflush()

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
>> >
>> > This has a slight semantic behavioural change.
>> 
>> Yeap.
>> 
>> See the answer to Peter.  But three things came to mind:
>> 
>> a - the size of the buffer is small (between 32KB and 256KB depending
>>     how you count it).  So we are going to call qemu_fflush() really
>>     soon.
>> 
>> b - We are using this value to calculate how much we can send through
>>     the wire.  Here we are saything how much we have accepted to send.
>> 
>> c - When using multifd the number of bytes that we send through the qemu
>>     file is even smaller. migration-test multifd test send 300MB of data
>>     through multifd channels and around 300KB on the qemu_file channel.
>> 
>> 
>> >
>> > By accounting for rate limit in the qemu_put functions, we ensure
>> > that we stop growing the iovec when rate limiting activates.
>> >
>> > If we only apply rate limit in the the flush function, that will
>> > let the  f->iov continue to accumulate buffers, while we have
>> > rate limited the actual transfer.
>> 
>> 256KB maximum.  Our accounting has bigger errors than that.
>> 
>> 
>> > This makes me uneasy - it feels like a bad idea to continue to
>> > accumulate buffers if we're not ready to send them
>> 
>> I still think that the change is correct.  But as you and Peter have
>> concerns about it, I will think a bit more about it.
>
> If Peter's calculations are correct, then I don't have any objection,
> as that's a small overhead.

#define IOV_MAX 1024
....
#define IO_BUF_SIZE 32768
#define MAX_IOV_SIZE MIN_CONST(IOV_MAX, 64)

struct QEMUFile {
   ...
    uint8_t buf[IO_BUF_SIZE];

    struct iovec iov[MAX_IOV_SIZE];
    ....
}

Later, Juan.