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

[Peter Xu <peterx@redhat.com>]

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 <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Paul Durrant <paul@xen.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Hyman Huang <yong.huang@smartx.com>
> Reviewed-by: Hyman Huang <yong.huang@smartx.com>
> Signed-off-by: Cédric Le Goater <clg@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.

> +            }
>              migration_bitmap_sync_precopy(rs, false);
>          }
>      }
> +out_unlock:
>      qemu_mutex_unlock_ramlist();
>  
> +    if (!ret) {
> +        return;
> +    }
> +
>      /*
>       * After an eventual first bitmap sync, fixup the initial bitmap
>       * containing all 1s to exclude any discarded pages from migration.
> @@ -3631,6 +3644,8 @@ int colo_init_ram_cache(void)
>  void colo_incoming_start_dirty_log(void)
>  {
>      RAMBlock *block = NULL;
> +    Error *local_err = NULL;
> +
>      /* For memory_global_dirty_log_start below. */
>      bql_lock();
>      qemu_mutex_lock_ramlist();
> @@ -3642,7 +3657,10 @@ void colo_incoming_start_dirty_log(void)
>              /* Discard this dirty bitmap record */
>              bitmap_zero(block->bmap, block->max_length >> TARGET_PAGE_BITS);
>          }
> -        memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION);
> +        if (!memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION,
> +                                           &local_err)) {
> +            error_report_err(local_err);
> +        }
>      }
>      ram_state->migration_dirty_pages = 0;
>      qemu_mutex_unlock_ramlist();
> diff --git a/system/memory.c b/system/memory.c
> index 3600e716149407c10a1f6bf8f0a81c2611cf15ba..cbc098216b789f50460f1d1bc7ec122030693d9e 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2931,10 +2931,9 @@ static void memory_global_dirty_log_rollback(MemoryListener *listener,
>      }
>  }
>  
> -void memory_global_dirty_log_start(unsigned int flags)
> +bool memory_global_dirty_log_start(unsigned int flags, Error **errp)
>  {
>      unsigned int old_flags;
> -    Error *local_err = NULL;
>  
>      assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
>  
> @@ -2946,7 +2945,7 @@ void memory_global_dirty_log_start(unsigned int flags)
>  
>      flags &= ~global_dirty_tracking;
>      if (!flags) {
> -        return;
> +        return true;
>      }
>  
>      old_flags = global_dirty_tracking;
> @@ -2959,7 +2958,7 @@ void memory_global_dirty_log_start(unsigned int flags)
>  
>          QTAILQ_FOREACH(listener, &memory_listeners, link) {
>              if (listener->log_global_start) {
> -                ret = listener->log_global_start(listener, &local_err);
> +                ret = listener->log_global_start(listener, errp);
>                  if (!ret) {
>                      break;
>                  }
> @@ -2969,14 +2968,14 @@ void memory_global_dirty_log_start(unsigned int flags)
>          if (!ret) {
>              memory_global_dirty_log_rollback(QTAILQ_PREV(listener, link),
>                                               flags);
> -            error_report_err(local_err);
> -            return;
> +            return false;
>          }
>  
>          memory_region_transaction_begin();
>          memory_region_update_pending = true;
>          memory_region_transaction_commit();
>      }
> +    return true;
>  }
>  
>  static void memory_global_dirty_log_do_stop(unsigned int flags)
> -- 
> 2.44.0
> 

-- 
Peter Xu