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;
+ }
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
On Wed, Mar 6, 2024 at 9:35 PM Cédric Le Goater <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? 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 <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;
> + }
> 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
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
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
>> --- 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.
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
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 <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.
ok. will do.
Thanks,
C.
>> --- 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.
© 2016 - 2024 Red Hat, Inc.