On Thu, Mar 21, 2024 at 12:20:32PM -0400, Peter Xu wrote:
> On Wed, Mar 20, 2024 at 05:44:53PM -0400, pet...@redhat.com wrote:
> > From: Peter Xu <pet...@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 <n...@linux.ibm.com>
> > Fixes: c8df4a7aef ("migration: Split save_live_pending() into
> > state_pending_*")
> > Signed-off-by: Peter Xu <pet...@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 <pet...@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 <n...@linux.ibm.com>
Fixes: b0504edd40 ("migration: Drop unnecessary check in ram's pending_exact()")
Reviewed-by: Fabiano Rosas <faro...@suse.de>
Link: https://lore.kernel.org/r/20240320214453.584374-1-pet...@redhat.com
Signed-off-by: Peter Xu <pet...@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);
--
2.44.0
--
Peter Xu
