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

["Cédric Le Goater" <clg@redhat.com>]

On 3/16/24 03:41, Yong Huang wrote:
> 
> 
> On Wed, Mar 6, 2024 at 9:35 PM Cédric Le Goater <clg@redhat.com <mailto:clg@redhat.com>> 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
> 
> Hi, Cédric Le Goater. Could the functional modification be made
> separately from the patch? 

Are you suggesting one patch to add the Error ** parameter and a second
to report the error if there is a failure ? From the moment the prototype
is modified, all handlers need to take the change into account to avoid
a build break. Looks difficult.

> And my "Reviewed-by" is attached
> to the first patch that refines memory_global_dirty_log_start's
> function declaration.

OK. I should resend a v5 anyhow, I will remove your R-b and you can
reconsider.

Thanks,

C.


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