| Message ID | 20240320214453.584374-1-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration/postcopy: Fix high frequency sync | expand |
peterx@redhat.com writes: > From: Peter Xu <peterx@redhat.com> > > On current code base I can observe extremely high sync count during > precopy, as long as one enables postcopy-ram=on before switchover to > postcopy. > > To provide some context of when we decide to do a full sync: we check > must_precopy (which implies "data must be sent during precopy phase"), and > as long as it is lower than the threshold size we calculated (out of > bandwidth and expected downtime) we will kick off the slow sync. > > However, when postcopy is enabled (even if still during precopy phase), RAM > only reports all pages as can_postcopy, and report must_precopy==0. Then > "must_precopy <= threshold_size" mostly always triggers and enforces a slow > sync for every call to migration_iteration_run() when postcopy is enabled > even if not used. That is insane. > > It turns out it was a regress bug introduced in the previous refactoring in > QEMU 8.0 in late 2022. Fix this by checking the whole RAM size rather than > must_precopy, like before. Not copy stable yet as many things changed, and > even if this should be a major performance regression, no functional change > has observed (and that's also probably why nobody found it). I only notice > this when looking for another bug reported by Nina. > > When at it, cleanup a little bit on the lines around. > > Cc: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > Fixes: c8df4a7aef ("migration: Split save_live_pending() into state_pending_*") > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de> > --- > > Nina: I copied you only because this might still be relevant, as this issue > also misteriously points back to c8df4a7aef.. However I don't think it > should be a fix of your problem, at most it can change the possibility of > reproducability. > > This is not a regression for this release, but I still want to have it for > 9.0. Fabiano, any opinions / objections? Go for it. > --- > migration/migration.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 047b6b49cf..9fe8fd2afd 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -3199,17 +3199,16 @@ typedef enum { > */ > static MigIterateState migration_iteration_run(MigrationState *s) > { > - uint64_t must_precopy, can_postcopy; > + uint64_t must_precopy, can_postcopy, pending_size; > Error *local_err = NULL; > bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE; > bool can_switchover = migration_can_switchover(s); > > qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy); > - uint64_t pending_size = must_precopy + can_postcopy; > - > + pending_size = must_precopy + can_postcopy; > trace_migrate_pending_estimate(pending_size, must_precopy, can_postcopy); > > - if (must_precopy <= s->threshold_size) { > + if (pending_size < s->threshold_size) { > qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy); > pending_size = must_precopy + can_postcopy; > trace_migrate_pending_exact(pending_size, must_precopy, can_postcopy);
On Wed, Mar 20, 2024 at 05:44:53PM -0400, peterx@redhat.com wrote: > From: Peter Xu <peterx@redhat.com> > > On current code base I can observe extremely high sync count during > precopy, as long as one enables postcopy-ram=on before switchover to > postcopy. > > To provide some context of when we decide to do a full sync: we check > must_precopy (which implies "data must be sent during precopy phase"), and > as long as it is lower than the threshold size we calculated (out of > bandwidth and expected downtime) we will kick off the slow sync. > > However, when postcopy is enabled (even if still during precopy phase), RAM > only reports all pages as can_postcopy, and report must_precopy==0. Then > "must_precopy <= threshold_size" mostly always triggers and enforces a slow > sync for every call to migration_iteration_run() when postcopy is enabled > even if not used. That is insane. > > It turns out it was a regress bug introduced in the previous refactoring in > QEMU 8.0 in late 2022. Fix this by checking the whole RAM size rather than > must_precopy, like before. Not copy stable yet as many things changed, and > even if this should be a major performance regression, no functional change > has observed (and that's also probably why nobody found it). I only notice > this when looking for another bug reported by Nina. > > When at it, cleanup a little bit on the lines around. > > Cc: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > Fixes: c8df4a7aef ("migration: Split save_live_pending() into state_pending_*") > Signed-off-by: Peter Xu <peterx@redhat.com> queued for 9.0-rc1.
On Thu, Mar 21, 2024 at 12:20:32PM -0400, Peter Xu wrote: > On Wed, Mar 20, 2024 at 05:44:53PM -0400, peterx@redhat.com wrote: > > From: Peter Xu <peterx@redhat.com> > > > > On current code base I can observe extremely high sync count during > > precopy, as long as one enables postcopy-ram=on before switchover to > > postcopy. > > > > To provide some context of when we decide to do a full sync: we check > > must_precopy (which implies "data must be sent during precopy phase"), and > > as long as it is lower than the threshold size we calculated (out of > > bandwidth and expected downtime) we will kick off the slow sync. > > > > However, when postcopy is enabled (even if still during precopy phase), RAM > > only reports all pages as can_postcopy, and report must_precopy==0. Then > > "must_precopy <= threshold_size" mostly always triggers and enforces a slow > > sync for every call to migration_iteration_run() when postcopy is enabled > > even if not used. That is insane. > > > > It turns out it was a regress bug introduced in the previous refactoring in > > QEMU 8.0 in late 2022. Fix this by checking the whole RAM size rather than > > must_precopy, like before. Not copy stable yet as many things changed, and > > even if this should be a major performance regression, no functional change > > has observed (and that's also probably why nobody found it). I only notice > > this when looking for another bug reported by Nina. > > > > When at it, cleanup a little bit on the lines around. > > > > Cc: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > Fixes: c8df4a7aef ("migration: Split save_live_pending() into state_pending_*") > > Signed-off-by: Peter Xu <peterx@redhat.com> > > queued for 9.0-rc1. When I was testing today on an old 8.2.0 binary I found that it's actually working all fine.. It's because 28ef5339c3 ("migration: fix ram_state_pending_exact()") actually fixed exactly the same issue, though that was a partial fix, which I'll consider it as a "workaround" (because it only fixed RAM, while the issue lies in the core calculations), which was overlooked in the cleanup patch I did.. This patch should provide the complete fix. I didn't check whether other iterators can be affected, though. To make it clearer, I'll change the Fixes to point to my cleanup patch, as that's indeed the first commit to expose this issue again at least for a generic postcopy use case (aka, my fault to break it..). Then it also means stable branches are all fine. I also rewrote the commit log. Attaching the updated version here just for reference (no code changes). ====8<==== From 32e3146be16fef9d0fe7b0818265c9d07bb51de3 Mon Sep 17 00:00:00 2001 From: Peter Xu <peterx@redhat.com> Date: Wed, 20 Mar 2024 17:44:53 -0400 Subject: [PATCH] migration/postcopy: Fix high frequency sync With current code base I can observe extremely high sync count during precopy, as long as one enables postcopy-ram=on before switchover to postcopy. To provide some context of when QEMU decides to do a full sync: it checks must_precopy (which implies "data must be sent during precopy phase"), and as long as it is lower than the threshold size we calculated (out of bandwidth and expected downtime) QEMU will kick off the slow/exact sync. However, when postcopy is enabled (even if still during precopy phase), RAM only reports all pages as can_postcopy, and report must_precopy==0. Then "must_precopy <= threshold_size" mostly always triggers and enforces a slow sync for every call to migration_iteration_run() when postcopy is enabled even if not used. That is insane. It turns out it was a regress bug introduced in the previous refactoring in 8.0 as reported by Nina [1]: (a) c8df4a7aef ("migration: Split save_live_pending() into state_pending_*") Then a workaround patch is applied at the end of release (8.0-rc4) to fix it: (b) 28ef5339c3 ("migration: fix ram_state_pending_exact()") However that "workaround" was overlooked when during the cleanup in this 9.0 release in this commit.. (c) b0504edd40 ("migration: Drop unnecessary check in ram's pending_exact()") Then the issue was re-exposed as reported by Nina [1]. The problem with (b) is that it only fixed the case for RAM, rather than all the rest of iterators. Here a slow sync should only be required if all dirty data (precopy+postcopy) is less than the threshold_size that QEMU calculated. It is even debatable whether a sync is needed when switched to postcopy. Currently ram_state_pending_exact() will be mostly noop if switched to postcopy, and that logic seems to apply too for all the rest of iterators, as sync dirty bitmap during a postcopy doesn't make much sense. However let's leave such change for later, as we're in rc phase. So rather than reusing commit (b), this patch provides the complete fix for all iterators. When at it, cleanup a little bit on the lines around. [1] https://gitlab.com/qemu-project/qemu/-/issues/1565 Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> Fixes: b0504edd40 ("migration: Drop unnecessary check in ram's pending_exact()") Reviewed-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20240320214453.584374-1-peterx@redhat.com Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/migration/migration.c b/migration/migration.c index 047b6b49cf..9fe8fd2afd 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -3199,17 +3199,16 @@ typedef enum { */ static MigIterateState migration_iteration_run(MigrationState *s) { - uint64_t must_precopy, can_postcopy; + uint64_t must_precopy, can_postcopy, pending_size; Error *local_err = NULL; bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE; bool can_switchover = migration_can_switchover(s); qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy); - uint64_t pending_size = must_precopy + can_postcopy; - + pending_size = must_precopy + can_postcopy; trace_migrate_pending_estimate(pending_size, must_precopy, can_postcopy); - if (must_precopy <= s->threshold_size) { + if (pending_size < s->threshold_size) { qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy); pending_size = must_precopy + can_postcopy; trace_migrate_pending_exact(pending_size, must_precopy, can_postcopy);
diff --git a/migration/migration.c b/migration/migration.c index 047b6b49cf..9fe8fd2afd 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -3199,17 +3199,16 @@ typedef enum { */ static MigIterateState migration_iteration_run(MigrationState *s) { - uint64_t must_precopy, can_postcopy; + uint64_t must_precopy, can_postcopy, pending_size; Error *local_err = NULL; bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE; bool can_switchover = migration_can_switchover(s); qemu_savevm_state_pending_estimate(&must_precopy, &can_postcopy); - uint64_t pending_size = must_precopy + can_postcopy; - + pending_size = must_precopy + can_postcopy; trace_migrate_pending_estimate(pending_size, must_precopy, can_postcopy); - if (must_precopy <= s->threshold_size) { + if (pending_size < s->threshold_size) { qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy); pending_size = must_precopy + can_postcopy; trace_migrate_pending_exact(pending_size, must_precopy, can_postcopy);