Re: [PATCH v4 14/25] memory: Add Error** argument to the global_dirty_log routines
On Mon, Mar 18, 2024 at 05:08:13PM +0100, Cédric Le Goater wrote: > > > --- 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(); > > btw, should we use bitmap_try_new() to create the bitmaps instead of > bitmap_new() which can abort() ? I'm not sure how much it'll help in reality; if allocation can fail here I would expect qemu crash sooner or later.. but I agree the try_new() seems reasonable too to be used here if this can fail now, after all migration is extra feature on top of VM's emulation functions, so it's optional. Thanks, -- Peter Xu
Re: [PATCH v4 14/25] memory: Add Error** argument to the global_dirty_log routines
On 3/16/24 03:41, Yong Huang wrote: On Wed, Mar 6, 2024 at 9:35 PM Cédric Le Goater mailto:c...@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 mailto:sstabell...@kernel.org>> Cc: Anthony Perard mailto:anthony.per...@citrix.com>> Cc: Paul Durrant mailto:p...@xen.org>> Cc: "Michael S. Tsirkin" mailto:m...@redhat.com>> Cc: Paolo Bonzini mailto:pbonz...@redhat.com>> Cc: David Hildenbrand mailto:da...@redhat.com>> Cc: Hyman Huang mailto:yong.hu...@smartx.com>> Reviewed-by: Hyman Huang mailto:yong.hu...@smartx.com>> Signed-off-by: Cédric Le Goater mailto: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 567bc4c9fdb53e8f63487f1400980275687d..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..
Re: [PATCH v4 14/25] memory: Add Error** argument to the global_dirty_log routines
--- 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(); btw, should we use bitmap_try_new() to create the bitmaps instead of bitmap_new() which can abort() ? /* 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(). C.
Re: [PATCH v4 14/25] memory: Add Error** argument to the global_dirty_log routines
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 Cc: Anthony Perard Cc: Paul Durrant Cc: "Michael S. Tsirkin" Cc: Paolo Bonzini Cc: David Hildenbrand Cc: Hyman Huang Reviewed-by: Hyman Huang Signed-off-by: Cédric Le Goater --- 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 567bc4c9fdb53e8f63487f1400980275687d..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
Re: [PATCH v4 14/25] memory: Add Error** argument to the global_dirty_log routines
--- 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. Let's do that in a new prereq patch. I will change ram_state_init() and xbzrle_init() to take an Error ** argument while at it. Thanks, C.
Re: [PATCH v4 14/25] memory: Add Error** argument to the global_dirty_log routines
On Wed, Mar 6, 2024 at 9:35 PM 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 > Hi, Cédric Le Goater. Could the functional modification be made separately from the patch? And my "Reviewed-by" is attached to the first patch that refines memory_global_dirty_log_start's function declaration. > 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 > Cc: Anthony Perard > Cc: Paul Durrant > Cc: "Michael S. Tsirkin" > Cc: Paolo Bonzini > Cc: David Hildenbrand > Cc: Hyman Huang > Reviewed-by: Hyman Huang > Signed-off-by: Cédric Le Goater > --- > > 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 > 567bc4c9fdb53e8f63487f1400980275687d..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; > +
Re: [PATCH v4 14/25] memory: Add Error** argument to the global_dirty_log routines
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 > Cc: Anthony Perard > Cc: Paul Durrant > Cc: "Michael S. Tsirkin" > Cc: Paolo Bonzini > Cc: David Hildenbrand > Cc: Hyman Huang > Reviewed-by: Hyman Huang > Signed-off-by: Cédric Le Goater > --- > > 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 > 567bc4c9fdb53e8f63487f1400980275687d..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 cre
[PATCH v4 14/25] memory: Add Error** argument to the global_dirty_log routines
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 Cc: Anthony Perard Cc: Paul Durrant Cc: "Michael S. Tsirkin" Cc: Paolo Bonzini Cc: David Hildenbrand Cc: Hyman Huang Reviewed-by: Hyman Huang Signed-off-by: Cédric Le Goater --- 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 567bc4c9fdb53e8f63487f1400980275687d..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_dir