[PATCH v6 0/3] Migration: Make more ram_counters atomic

[PATCH v6 0/3] Migration: Make more ram_counters atomic

[Juan Quintela]

Hi

Changes in v6:
- Create stat64_set() operation
  Impersonating Paolo in "Author" field, as he is the real author.
- Update to stat64_set()
- Everything is consistent and coherent under the sun.

Please review.

[v5]
Not only change the type of the counters, also use the __nocheck()
variants of the functions.

Please, review.

[v4]
- Change aligned_uint64_t to size_t to make (some) 32bit hosts happy.

Please review.

[v3]
- Addressed reviews
- All counters are now atomic, either Stat64 or atomic.
- Rename duplicated to zero_pages
- Rename normal to zero_pages.

Please review.

[v2]
- fix typos found by David Edmondson
- Add review-by tags.

Please review.

[v1]
On previous series we cerate ram_atomic_counters.  But we basically
need that all counters are atomic.  So move back to only have
ram_counters, just with a new type that allows the atomic counters.

Once there, move update of stats out of RAM mutex.
And make multifd_bytes atomic.

Later, Juan.

Juan Quintela (2):
  migration: Make dirty_pages_rate atomic
  migration: Make dirty_bytes_last_sync atomic

Paolo Bonzini (1):
  stat64: Add stat64_set() operation

 include/qemu/stats64.h |  6 ++++++
 migration/migration.c  |  9 ++++++---
 migration/ram.c        |  7 ++++---
 migration/ram.h        |  4 ++--
 util/stats64.c         | 11 +++++++++++
 5 files changed, 29 insertions(+), 8 deletions(-)

-- 
2.40.0

[PATCH v6 1/3] stat64: Add stat64_set() operation

[Juan Quintela]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/qemu/stats64.h |  6 ++++++
 util/stats64.c         | 11 +++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/qemu/stats64.h b/include/qemu/stats64.h
index 802402254b..99b5cb724a 100644
--- a/include/qemu/stats64.h
+++ b/include/qemu/stats64.h
@@ -40,6 +40,11 @@ static inline uint64_t stat64_get(const Stat64 *s)
     return qatomic_read__nocheck(&s->value);
 }
 
+static inline void stat64_set(Stat64 *s, uint64_t value)
+{
+    qatomic_set__nocheck(&s->value, value);
+}
+
 static inline void stat64_add(Stat64 *s, uint64_t value)
 {
     qatomic_add(&s->value, value);
@@ -62,6 +67,7 @@ static inline void stat64_max(Stat64 *s, uint64_t value)
 }
 #else
 uint64_t stat64_get(const Stat64 *s);
+void stat64_set(Stat64 *s, uint64_t value);
 bool stat64_min_slow(Stat64 *s, uint64_t value);
 bool stat64_max_slow(Stat64 *s, uint64_t value);
 bool stat64_add32_carry(Stat64 *s, uint32_t low, uint32_t high);
diff --git a/util/stats64.c b/util/stats64.c
index 897613c949..09736014ec 100644
--- a/util/stats64.c
+++ b/util/stats64.c
@@ -57,6 +57,17 @@ uint64_t stat64_get(const Stat64 *s)
     return ((uint64_t)high << 32) | low;
 }
 
+void stat64_set(Stat64 *s, uint64_t val)
+{
+    while (!stat64_wrtrylock(s)) {
+        cpu_relax();
+    }
+
+    qatomic_set(&s->high, val >> 32);
+    qatomic_set(&s->low, val);
+    stat64_wrunlock(s);
+}
+
 bool stat64_add32_carry(Stat64 *s, uint32_t low, uint32_t high)
 {
     uint32_t old;
-- 
2.40.0

Re: [PATCH v6 1/3] stat64: Add stat64_set() operation

[Richard Henderson]

On 4/27/23 10:52, Juan Quintela wrote:
> From: Paolo Bonzini<pbonzini@redhat.com>
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> Reviewed-by: Juan Quintela<quintela@redhat.com>
> Signed-off-by: Juan Quintela<quintela@redhat.com>
> ---
>   include/qemu/stats64.h |  6 ++++++
>   util/stats64.c         | 11 +++++++++++
>   2 files changed, 17 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v6 2/3] migration: Make dirty_pages_rate atomic

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>

---

Don't use __nocheck() variants
Use stat64_get()
---
 migration/migration.c | 6 ++++--
 migration/ram.c       | 5 +++--
 migration/ram.h       | 2 +-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 22e8586623..900dfb6855 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1005,7 +1005,8 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
 
     if (s->state != MIGRATION_STATUS_COMPLETED) {
         info->ram->remaining = ram_bytes_remaining();
-        info->ram->dirty_pages_rate = ram_counters.dirty_pages_rate;
+        info->ram->dirty_pages_rate =
+           stat64_get(&ram_counters.dirty_pages_rate);
     }
 }
 
@@ -2751,7 +2752,8 @@ static void migration_update_counters(MigrationState *s,
      * if we haven't sent anything, we don't want to
      * recalculate. 10000 is a small enough number for our purposes
      */
-    if (ram_counters.dirty_pages_rate && transferred > 10000) {
+    if (stat64_get(&ram_counters.dirty_pages_rate) &&
+        transferred > 10000) {
         s->expected_downtime = ram_counters.remaining / bandwidth;
     }
 
diff --git a/migration/ram.c b/migration/ram.c
index 01356f60a4..fa94c99766 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1129,8 +1129,9 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
     double compressed_size;
 
     /* calculate period counters */
-    ram_counters.dirty_pages_rate = rs->num_dirty_pages_period * 1000
-                / (end_time - rs->time_last_bitmap_sync);
+    stat64_set(&ram_counters.dirty_pages_rate,
+               rs->num_dirty_pages_period * 1000 /
+               (end_time - rs->time_last_bitmap_sync));
 
     if (!page_count) {
         return;
diff --git a/migration/ram.h b/migration/ram.h
index a6e0d70226..f189cc79f8 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -41,7 +41,7 @@
  * one thread).
  */
 typedef struct {
-    int64_t dirty_pages_rate;
+    Stat64 dirty_pages_rate;
     Stat64 dirty_sync_count;
     Stat64 dirty_sync_missed_zero_copy;
     Stat64 downtime_bytes;
-- 
2.40.0

Re: [PATCH v6 2/3] migration: Make dirty_pages_rate atomic

[Richard Henderson]

On 4/27/23 10:52, Juan Quintela wrote:
> Signed-off-by: Juan Quintela<quintela@redhat.com>
> Reviewed-by: Peter Xu<peterx@redhat.com>
> 
> ---
> 
> Don't use __nocheck() variants
> Use stat64_get()
> ---
>   migration/migration.c | 6 ++++--
>   migration/ram.c       | 5 +++--
>   migration/ram.h       | 2 +-
>   3 files changed, 8 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v6 3/3] migration: Make dirty_bytes_last_sync atomic

[Juan Quintela]

As we set its value, it needs to be operated with atomics.
We rename it from remaining to better reflect its meaning.

Statistics always return the real reamaining bytes.  This was used to
store how much pages where dirty on the previous generation, so we can
calculate the expected downtime as: dirty_bytes_last_sync /
current_bandwith.

If we use the actual remaining bytes, we would see a very small value
at the end of the iteration.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>

---

I am open to use ram_bytes_remaining() in its only use and be more
"optimistic" about the downtime.

Don't use __nocheck() functions.
Use stat64_get() now that it exists.
---
 migration/migration.c | 3 ++-
 migration/ram.c       | 2 +-
 migration/ram.h       | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 900dfb6855..cc09a88aac 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2754,7 +2754,8 @@ static void migration_update_counters(MigrationState *s,
      */
     if (stat64_get(&ram_counters.dirty_pages_rate) &&
         transferred > 10000) {
-        s->expected_downtime = ram_counters.remaining / bandwidth;
+        s->expected_downtime =
+            stat64_get(&ram_counters.dirty_bytes_last_sync) / bandwidth;
     }
 
     qemu_file_reset_rate_limit(s->to_dst_file);
diff --git a/migration/ram.c b/migration/ram.c
index fa94c99766..fcdcf78ec7 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1223,7 +1223,7 @@ static void migration_bitmap_sync(RAMState *rs)
         RAMBLOCK_FOREACH_NOT_IGNORED(block) {
             ramblock_sync_dirty_bitmap(rs, block);
         }
-        ram_counters.remaining = ram_bytes_remaining();
+        stat64_set(&ram_counters.dirty_bytes_last_sync, ram_bytes_remaining());
     }
     qemu_mutex_unlock(&rs->bitmap_mutex);
 
diff --git a/migration/ram.h b/migration/ram.h
index f189cc79f8..04b05e1b2c 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -41,6 +41,7 @@
  * one thread).
  */
 typedef struct {
+    Stat64 dirty_bytes_last_sync;
     Stat64 dirty_pages_rate;
     Stat64 dirty_sync_count;
     Stat64 dirty_sync_missed_zero_copy;
@@ -51,7 +52,6 @@ typedef struct {
     Stat64 postcopy_bytes;
     Stat64 postcopy_requests;
     Stat64 precopy_bytes;
-    int64_t remaining;
     Stat64 transferred;
 } RAMStats;
 
-- 
2.40.0

Re: [PATCH v6 3/3] migration: Make dirty_bytes_last_sync atomic

[Richard Henderson]

On 4/27/23 10:52, Juan Quintela wrote:
> As we set its value, it needs to be operated with atomics.
> We rename it from remaining to better reflect its meaning.
> 
> Statistics always return the real reamaining bytes.  This was used to
> store how much pages where dirty on the previous generation, so we can
> calculate the expected downtime as: dirty_bytes_last_sync /
> current_bandwith.
> 
> If we use the actual remaining bytes, we would see a very small value
> at the end of the iteration.
> 
> Signed-off-by: Juan Quintela<quintela@redhat.com>
> Reviewed-by: Peter Xu<peterx@redhat.com>
> 
> ---
> 
> I am open to use ram_bytes_remaining() in its only use and be more
> "optimistic" about the downtime.
> 
> Don't use __nocheck() functions.
> Use stat64_get() now that it exists.
> ---
>   migration/migration.c | 3 ++-
>   migration/ram.c       | 2 +-
>   migration/ram.h       | 2 +-
>   3 files changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~