Re: [PATCH v4 14/25] memory: Add Error** argument to the global_dirty_log routines

[Cédric Le Goater]

On 3/15/24 12:34, Peter Xu wrote:

On Wed, Mar 06, 2024 at 02:34:29PM +0100, Cédric Le Goater wrote:

Now that the log_global*() handlers take an Error** parameter and
return a bool, do the same for memory_global_dirty_log_start() and
memory_global_dirty_log_stop(). The error is reported in the callers
for now and it will be propagated in the call stack in the next
changes.

To be noted a functional change in ram_init_bitmaps(), if the dirty
pages logger fails to start, there is no need to synchronize the dirty
pages bitmaps. colo_incoming_start_dirty_log() could be modified in a
similar way.

Cc: Stefano Stabellini <sstabell...@kernel.org>
Cc: Anthony Perard <anthony.per...@citrix.com>
Cc: Paul Durrant <p...@xen.org>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: David Hildenbrand <da...@redhat.com>
Cc: Hyman Huang <yong.hu...@smartx.com>
Reviewed-by: Hyman Huang <yong.hu...@smartx.com>
Signed-off-by: Cédric Le Goater <c...@redhat.com>
---

  Changes in v4:

  - Dropped log_global_stop() and log_global_sync() changes

include/exec/memory.h | 5 ++++-

  hw/i386/xen/xen-hvm.c |  2 +-
  migration/dirtyrate.c | 13 +++++++++++--
  migration/ram.c       | 22 ++++++++++++++++++++--
  system/memory.c       | 11 +++++------
  5 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 
5555567bc4c9fdb53e8f63487f1400980275687d..c129ee6db7162504bd72d4cfc69b5affb2cd87e8
 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2570,8 +2570,11 @@ void memory_listener_unregister(MemoryListener 
*listener);
   * memory_global_dirty_log_start: begin dirty logging for all regions
   *
   * @flags: purpose of starting dirty log, migration or dirty rate
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: true on success, else false setting @errp with error.
   */
-void memory_global_dirty_log_start(unsigned int flags);
+bool memory_global_dirty_log_start(unsigned int flags, Error **errp);

/**

   * memory_global_dirty_log_stop: end dirty logging for all regions
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 
0608ca99f5166fd6379ee674442484e805eff9c0..57cb7df50788a6c31eff68c95e8eaa856fdebede
 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -654,7 +654,7 @@ void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t 
length)
  void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
  {
      if (enable) {
-        memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
+        memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION, errp);
      } else {
          memory_global_dirty_log_stop(GLOBAL_DIRTY_MIGRATION);
      }
diff --git a/migration/dirtyrate.c b/migration/dirtyrate.c
index 
1d2e85746fb7b10eb7f149976970f9a92125af8a..d02d70b7b4b86a29d4d5540ded416543536d8f98
 100644
--- a/migration/dirtyrate.c
+++ b/migration/dirtyrate.c
@@ -90,9 +90,15 @@ static int64_t do_calculate_dirtyrate(DirtyPageRecord 
dirty_pages,

void global_dirty_log_change(unsigned int flag, bool start)

  {
+    Error *local_err = NULL;
+    bool ret;
+
      bql_lock();
      if (start) {
-        memory_global_dirty_log_start(flag);
+        ret = memory_global_dirty_log_start(flag, &local_err);
+        if (!ret) {
+            error_report_err(local_err);
+        }
      } else {
          memory_global_dirty_log_stop(flag);
      }
@@ -608,9 +614,12 @@ static void calculate_dirtyrate_dirty_bitmap(struct 
DirtyRateConfig config)
  {
      int64_t start_time;
      DirtyPageRecord dirty_pages;
+    Error *local_err = NULL;

bql_lock();

-    memory_global_dirty_log_start(GLOBAL_DIRTY_DIRTY_RATE);
+    if (!memory_global_dirty_log_start(GLOBAL_DIRTY_DIRTY_RATE, &local_err)) {
+        error_report_err(local_err);
+    }

/*

       * 1'round of log sync may return all 1 bits with
diff --git a/migration/ram.c b/migration/ram.c
index 
c5149b7d717aefad7f590422af0ea4a40e7507be..397b4c0f218a66d194e44f9c5f9fe8e9885c48b6
 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2836,18 +2836,31 @@ static void 
migration_bitmap_clear_discarded_pages(RAMState *rs)

static void ram_init_bitmaps(RAMState *rs)

  {
+    Error *local_err = NULL;
+    bool ret = true;
+
      qemu_mutex_lock_ramlist();

WITH_RCU_READ_LOCK_GUARD() {

          ram_list_init_bitmaps();
          /* We don't use dirty log with background snapshots */
          if (!migrate_background_snapshot()) {
-            memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
+            ret = memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION,
+                                                &local_err);
+            if (!ret) {
+                error_report_err(local_err);
+                goto out_unlock;

Here we may need to free the bitmaps created in ram_list_init_bitmaps().

We can have a helper ram_bitmaps_destroy() for that.

One thing be careful is the new file_bmap can be created but missing in the
ram_save_cleanup(), it's because it's freed earlier.  IMHO if we will have
a new ram_bitmaps_destroy() we can unconditionally free file_bmap there
too, as if it's freed early g_free() is noop.

ok. will do.


Thanks,

C.