[PATCH 0/3] migration: some small cleanups

[PATCH 0/3] migration: some small cleanups

[peterx]

From: Peter Xu <peterx@redhat.com>

I found three tiny little patches lying around probably for a long time,
sending it out before deleting the branch.  Review welcomed, thanks.

Peter Xu (3):
  migration: Make threshold_size an uint64_t
  migration: Drop unnecessary check in ram's pending_exact()
  analyze-migration.py: Remove trick on parsing ramblocks

 migration/migration.h        |  2 +-
 migration/ram.c              |  9 ++++-----
 scripts/analyze-migration.py | 11 +++--------
 3 files changed, 8 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH 1/3] migration: Make threshold_size an uint64_t

[peterx]

From: Peter Xu <peterx@redhat.com>

It's always used to compare against another uint64_t.  Make it always clear
that it's never a negative.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/migration.h b/migration/migration.h
index 17972dac34..a589ae8650 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -296,7 +296,7 @@ struct MigrationState {
      * this threshold; it's calculated from the requested downtime and
      * measured bandwidth, or avail-switchover-bandwidth if specified.
      */
-    int64_t threshold_size;
+    uint64_t threshold_size;
 
     /* params from 'migrate-set-parameters' */
     MigrationParameters parameters;
-- 
2.43.0

[PATCH 2/3] migration: Drop unnecessary check in ram's pending_exact()

[peterx]

From: Peter Xu <peterx@redhat.com>

When the migration frameworks fetches the exact pending sizes, it means
this check:

  remaining_size < s->threshold_size

Must have been done already, actually at migration_iteration_run():

    if (must_precopy <= s->threshold_size) {
        qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);

That should be after one round of ram_state_pending_estimate().  It makes
the 2nd check meaningless and can be dropped.

To say it in another way, when reaching ->state_pending_exact(), we
unconditionally sync dirty bits for precopy.

Then we can drop migrate_get_current() there too.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index c0cdcccb75..d5b7cd5ac2 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3213,21 +3213,20 @@ static void ram_state_pending_estimate(void *opaque, uint64_t *must_precopy,
 static void ram_state_pending_exact(void *opaque, uint64_t *must_precopy,
                                     uint64_t *can_postcopy)
 {
-    MigrationState *s = migrate_get_current();
     RAMState **temp = opaque;
     RAMState *rs = *temp;
+    uint64_t remaining_size;
 
-    uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
-
-    if (!migration_in_postcopy() && remaining_size < s->threshold_size) {
+    if (!migration_in_postcopy()) {
         bql_lock();
         WITH_RCU_READ_LOCK_GUARD() {
             migration_bitmap_sync_precopy(rs, false);
         }
         bql_unlock();
-        remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
     }
 
+    remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
+
     if (migrate_postcopy_ram()) {
         /* We can do postcopy, and all the data is postcopiable */
         *can_postcopy += remaining_size;
-- 
2.43.0

[PATCH 3/3] analyze-migration.py: Remove trick on parsing ramblocks

[peterx]

From: Peter Xu <peterx@redhat.com>

RAM_SAVE_FLAG_MEM_SIZE contains the total length of ramblock idstr to know
whether scanning of ramblocks is complete.  Drop the trick.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 scripts/analyze-migration.py | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index a39dfb8766..8a254a5b6a 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -151,17 +151,12 @@ def read(self):
             addr &= ~(self.TARGET_PAGE_SIZE - 1)
 
             if flags & self.RAM_SAVE_FLAG_MEM_SIZE:
-                while True:
+                total_length = addr
+                while total_length > 0:
                     namelen = self.file.read8()
-                    # We assume that no RAM chunk is big enough to ever
-                    # hit the first byte of the address, so when we see
-                    # a zero here we know it has to be an address, not the
-                    # length of the next block.
-                    if namelen == 0:
-                        self.file.file.seek(-1, 1)
-                        break
                     self.name = self.file.readstr(len = namelen)
                     len = self.file.read64()
+                    total_length -= len
                     self.sizeinfo[self.name] = '0x%016x' % len
                     if self.write_memory:
                         print(self.name)
-- 
2.43.0

Re: [PATCH 1/3] migration: Make threshold_size an uint64_t

[Philippe Mathieu-Daudé]

On 17/1/24 08:58, peterx@redhat.com wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> It's always used to compare against another uint64_t.  Make it always clear
> that it's never a negative.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   migration/migration.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/3] analyze-migration.py: Remove trick on parsing ramblocks

[Fabiano Rosas]

peterx@redhat.com writes:

> From: Peter Xu <peterx@redhat.com>
>
> RAM_SAVE_FLAG_MEM_SIZE contains the total length of ramblock idstr to know
> whether scanning of ramblocks is complete.  Drop the trick.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 2/3] migration: Drop unnecessary check in ram's pending_exact()

[Fabiano Rosas]

peterx@redhat.com writes:

> From: Peter Xu <peterx@redhat.com>
>
> When the migration frameworks fetches the exact pending sizes, it means
> this check:
>
>   remaining_size < s->threshold_size
>
> Must have been done already, actually at migration_iteration_run():
>
>     if (must_precopy <= s->threshold_size) {
>         qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);
>
> That should be after one round of ram_state_pending_estimate().  It makes
> the 2nd check meaningless and can be dropped.
>
> To say it in another way, when reaching ->state_pending_exact(), we
> unconditionally sync dirty bits for precopy.
>
> Then we can drop migrate_get_current() there too.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 1/3] migration: Make threshold_size an uint64_t

[Fabiano Rosas]

peterx@redhat.com writes:

> From: Peter Xu <peterx@redhat.com>
>
> It's always used to compare against another uint64_t.  Make it always clear
> that it's never a negative.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 0/3] migration: some small cleanups

[Peter Xu]

On Wed, Jan 17, 2024 at 03:58:45PM +0800, peterx@redhat.com wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> I found three tiny little patches lying around probably for a long time,
> sending it out before deleting the branch.  Review welcomed, thanks.
> 
> Peter Xu (3):
>   migration: Make threshold_size an uint64_t
>   migration: Drop unnecessary check in ram's pending_exact()
>   analyze-migration.py: Remove trick on parsing ramblocks
> 
>  migration/migration.h        |  2 +-
>  migration/ram.c              |  9 ++++-----
>  scripts/analyze-migration.py | 11 +++--------
>  3 files changed, 8 insertions(+), 14 deletions(-)

queued.

-- 
Peter Xu

Re: [PATCH 2/3] migration: Drop unnecessary check in ram's pending_exact()

[Nina Schoetterl-Glausch]

On Wed, 2024-01-17 at 15:58 +0800, peterx@redhat.com wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> When the migration frameworks fetches the exact pending sizes, it means
> this check:
> 
>   remaining_size < s->threshold_size
> 
> Must have been done already, actually at migration_iteration_run():
> 
>     if (must_precopy <= s->threshold_size) {
>         qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);
> 
> That should be after one round of ram_state_pending_estimate().  It makes
> the 2nd check meaningless and can be dropped.
> 
> To say it in another way, when reaching ->state_pending_exact(), we
> unconditionally sync dirty bits for precopy.
> 
> Then we can drop migrate_get_current() there too.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Hi Peter,

could you have a look at this issue:
https://gitlab.com/qemu-project/qemu/-/issues/1565

which I reopened. Previous thread here:

https://lore.kernel.org/qemu-devel/20230324184129.3119575-1-nsg@linux.ibm.com/

I'm seeing migration failures with s390x TCG again, which look the same to me
as those a while back.

> ---
>  migration/ram.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index c0cdcccb75..d5b7cd5ac2 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3213,21 +3213,20 @@ static void ram_state_pending_estimate(void *opaque, uint64_t *must_precopy,
>  static void ram_state_pending_exact(void *opaque, uint64_t *must_precopy,
>                                      uint64_t *can_postcopy)
>  {
> -    MigrationState *s = migrate_get_current();
>      RAMState **temp = opaque;
>      RAMState *rs = *temp;
> +    uint64_t remaining_size;
>  
> -    uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
> -
> -    if (!migration_in_postcopy() && remaining_size < s->threshold_size) {
> +    if (!migration_in_postcopy()) {
>          bql_lock();
>          WITH_RCU_READ_LOCK_GUARD() {
>              migration_bitmap_sync_precopy(rs, false);
>          }
>          bql_unlock();
> -        remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
>      }
>  
> +    remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
> +
>      if (migrate_postcopy_ram()) {
>          /* We can do postcopy, and all the data is postcopiable */
>          *can_postcopy += remaining_size;

This basically reverts 28ef5339c3 ("migration: fix ram_state_pending_exact()"), which originally
made the issue disappear.

Any thoughts on the matter appreciated.

Thanks,
Nina

Re: [PATCH 2/3] migration: Drop unnecessary check in ram's pending_exact()

[Nina Schoetterl-Glausch]

I cc'ed Juan, but it looks like he is no longer with Redhat.

Re: [PATCH 2/3] migration: Drop unnecessary check in ram's pending_exact()

[Peter Xu]

On Wed, Mar 20, 2024 at 06:51:26PM +0100, Nina Schoetterl-Glausch wrote:
> On Wed, 2024-01-17 at 15:58 +0800, peterx@redhat.com wrote:
> > From: Peter Xu <peterx@redhat.com>
> > 
> > When the migration frameworks fetches the exact pending sizes, it means
> > this check:
> > 
> >   remaining_size < s->threshold_size
> > 
> > Must have been done already, actually at migration_iteration_run():
> > 
> >     if (must_precopy <= s->threshold_size) {
> >         qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);
> > 
> > That should be after one round of ram_state_pending_estimate().  It makes
> > the 2nd check meaningless and can be dropped.
> > 
> > To say it in another way, when reaching ->state_pending_exact(), we
> > unconditionally sync dirty bits for precopy.
> > 
> > Then we can drop migrate_get_current() there too.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Hi Peter,

Hi, Nina,

> 
> could you have a look at this issue:
> https://gitlab.com/qemu-project/qemu/-/issues/1565
> 
> which I reopened. Previous thread here:
> 
> https://lore.kernel.org/qemu-devel/20230324184129.3119575-1-nsg@linux.ibm.com/
> 
> I'm seeing migration failures with s390x TCG again, which look the same to me
> as those a while back.

I'm still quite confused how that could be caused of this.

What you described in the previous bug report seems to imply some page was
leftover in migration so some page got corrupted after migrated.

However what this patch mostly does is it can sync more than before even if
I overlooked the condition check there (I still think the check is
redundant, there's one outlier when remaining_size == threshold_size, but I
don't think it should matter here as of now).  It'll make more sense if
this patch made the sync less, but that's not the case but vice versa.

> 
> > ---
> >  migration/ram.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/migration/ram.c b/migration/ram.c
> > index c0cdcccb75..d5b7cd5ac2 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -3213,21 +3213,20 @@ static void ram_state_pending_estimate(void *opaque, uint64_t *must_precopy,
> >  static void ram_state_pending_exact(void *opaque, uint64_t *must_precopy,
> >                                      uint64_t *can_postcopy)
> >  {
> > -    MigrationState *s = migrate_get_current();
> >      RAMState **temp = opaque;
> >      RAMState *rs = *temp;
> > +    uint64_t remaining_size;
> >  
> > -    uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
> > -
> > -    if (!migration_in_postcopy() && remaining_size < s->threshold_size) {
> > +    if (!migration_in_postcopy()) {
> >          bql_lock();
> >          WITH_RCU_READ_LOCK_GUARD() {
> >              migration_bitmap_sync_precopy(rs, false);
> >          }
> >          bql_unlock();
> > -        remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
> >      }
> >  
> > +    remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
> > +
> >      if (migrate_postcopy_ram()) {
> >          /* We can do postcopy, and all the data is postcopiable */
> >          *can_postcopy += remaining_size;
> 
> This basically reverts 28ef5339c3 ("migration: fix ram_state_pending_exact()"), which originally
> made the issue disappear.
> 
> Any thoughts on the matter appreciated.

In the previous discussion, you mentioned that you bisected to the commit
and also verified the fix.  Now you also mentioned in the bz that you can't
reporduce this bug manually.

Is it still possible to be reproduced with some scripts?  Do you also mean
that it's harder to reproduce comparing to before?  In all cases, some way
to reproduce it would definitely be helpful.

Even if we want to revert this change, we'll need to know whether this will
fix your case so we need something to verify it before a revert.  I'll
consider that the last though as I had a feeling this is papering over
something else.

Thanks,

-- 
Peter Xu

Re: [PATCH 2/3] migration: Drop unnecessary check in ram's pending_exact()

[Nina Schoetterl-Glausch]

On Wed, 2024-03-20 at 14:57 -0400, Peter Xu wrote:
> On Wed, Mar 20, 2024 at 06:51:26PM +0100, Nina Schoetterl-Glausch wrote:
> > On Wed, 2024-01-17 at 15:58 +0800, peterx@redhat.com wrote:
> > > From: Peter Xu <peterx@redhat.com>
> > > 
> > > When the migration frameworks fetches the exact pending sizes, it means
> > > this check:
> > > 
> > >   remaining_size < s->threshold_size
> > > 
> > > Must have been done already, actually at migration_iteration_run():
> > > 
> > >     if (must_precopy <= s->threshold_size) {
> > >         qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);
> > > 
> > > That should be after one round of ram_state_pending_estimate().  It makes
> > > the 2nd check meaningless and can be dropped.
> > > 
> > > To say it in another way, when reaching ->state_pending_exact(), we
> > > unconditionally sync dirty bits for precopy.
> > > 
> > > Then we can drop migrate_get_current() there too.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > 
> > Hi Peter,
> 
> Hi, Nina,
> 
> > 
> > could you have a look at this issue:
> > https://gitlab.com/qemu-project/qemu/-/issues/1565
> > 
> > which I reopened. Previous thread here:
> > 
> > https://lore.kernel.org/qemu-devel/20230324184129.3119575-1-nsg@linux.ibm.com/
> > 
> > I'm seeing migration failures with s390x TCG again, which look the same to me
> > as those a while back.
> 
> I'm still quite confused how that could be caused of this.
> 
> What you described in the previous bug report seems to imply some page was
> leftover in migration so some page got corrupted after migrated.
> 
> However what this patch mostly does is it can sync more than before even if
> I overlooked the condition check there (I still think the check is
> redundant, there's one outlier when remaining_size == threshold_size, but I
> don't think it should matter here as of now).  It'll make more sense if
> this patch made the sync less, but that's not the case but vice versa.

[...]

> In the previous discussion, you mentioned that you bisected to the commit
> and also verified the fix.  Now you also mentioned in the bz that you can't
> reporduce this bug manually.
> 
> Is it still possible to be reproduced with some scripts?  Do you also mean
> that it's harder to reproduce comparing to before?  In all cases, some way
> to reproduce it would definitely be helpful.

I tried running the kvm-unit-test a bunch of times in a loop and couldn't
trigger a failure. I just tried again on a different system and managed just
fine, yay. No idea why it wouldn't on the first system tho.
> 
> Even if we want to revert this change, we'll need to know whether this will
> fix your case so we need something to verify it before a revert.  I'll
> consider that the last though as I had a feeling this is papering over
> something else.

I can check if I can reproduce the issue before & after b0504edd ("migration:
Drop unnecessary check in ram's pending_exact()").
I can also check if I can reproduce it on x86, that worked last time.
Anything else? Ideas on how to pinpoint where the corruption happens?

> 
> Thanks,
> 

Re: [PATCH 2/3] migration: Drop unnecessary check in ram's pending_exact()

[Peter Xu]

On Wed, Mar 20, 2024 at 08:21:30PM +0100, Nina Schoetterl-Glausch wrote:
> On Wed, 2024-03-20 at 14:57 -0400, Peter Xu wrote:
> > On Wed, Mar 20, 2024 at 06:51:26PM +0100, Nina Schoetterl-Glausch wrote:
> > > On Wed, 2024-01-17 at 15:58 +0800, peterx@redhat.com wrote:
> > > > From: Peter Xu <peterx@redhat.com>
> > > > 
> > > > When the migration frameworks fetches the exact pending sizes, it means
> > > > this check:
> > > > 
> > > >   remaining_size < s->threshold_size
> > > > 
> > > > Must have been done already, actually at migration_iteration_run():
> > > > 
> > > >     if (must_precopy <= s->threshold_size) {
> > > >         qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);
> > > > 
> > > > That should be after one round of ram_state_pending_estimate().  It makes
> > > > the 2nd check meaningless and can be dropped.
> > > > 
> > > > To say it in another way, when reaching ->state_pending_exact(), we
> > > > unconditionally sync dirty bits for precopy.
> > > > 
> > > > Then we can drop migrate_get_current() there too.
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > 
> > > Hi Peter,
> > 
> > Hi, Nina,
> > 
> > > 
> > > could you have a look at this issue:
> > > https://gitlab.com/qemu-project/qemu/-/issues/1565
> > > 
> > > which I reopened. Previous thread here:
> > > 
> > > https://lore.kernel.org/qemu-devel/20230324184129.3119575-1-nsg@linux.ibm.com/
> > > 
> > > I'm seeing migration failures with s390x TCG again, which look the same to me
> > > as those a while back.
> > 
> > I'm still quite confused how that could be caused of this.
> > 
> > What you described in the previous bug report seems to imply some page was
> > leftover in migration so some page got corrupted after migrated.
> > 
> > However what this patch mostly does is it can sync more than before even if
> > I overlooked the condition check there (I still think the check is
> > redundant, there's one outlier when remaining_size == threshold_size, but I
> > don't think it should matter here as of now).  It'll make more sense if
> > this patch made the sync less, but that's not the case but vice versa.
> 
> [...]
> 
> > In the previous discussion, you mentioned that you bisected to the commit
> > and also verified the fix.  Now you also mentioned in the bz that you can't
> > reporduce this bug manually.
> > 
> > Is it still possible to be reproduced with some scripts?  Do you also mean
> > that it's harder to reproduce comparing to before?  In all cases, some way
> > to reproduce it would definitely be helpful.
> 
> I tried running the kvm-unit-test a bunch of times in a loop and couldn't
> trigger a failure. I just tried again on a different system and managed just
> fine, yay. No idea why it wouldn't on the first system tho.

There's probably still a bug somewhere.  If reproduction rate changed, it's
also a sign that it might not be directly relevant to this change, as
otherwise it should reproduce the same as before.

> > 
> > Even if we want to revert this change, we'll need to know whether this will
> > fix your case so we need something to verify it before a revert.  I'll
> > consider that the last though as I had a feeling this is papering over
> > something else.
> 
> I can check if I can reproduce the issue before & after b0504edd ("migration:
> Drop unnecessary check in ram's pending_exact()").
> I can also check if I can reproduce it on x86, that worked last time.
> Anything else? Ideas on how to pinpoint where the corruption happens?

I don't have a solid clue yet, but more information of the single case
where it reproduced could help.

I saw from the bug link that the cmdline is pretty simple.  However still
not sure of something that can be relevant.  E.g., did you use postcopy
(including when postcopy-ram enabled but precopy completed)?  Is there any
special device, like s390's CMMA (would that simplest cmdline include such
a device; apologies, I have zero knowledge there before today)?

I _think_ when reading the code I already found something quite unusual,
but only when postcopy is selected: I notice postcopy will frequently sync
dirty bitmap while it doesn't really necessarily need to, because
ram_state_pending_estimate() will report all ram as "can_postcopy"; it
means it's highly likely that this check will 99.999% always be true simply
because must_precopy can in most cases be zero:

    if (must_precopy <= s->threshold_size) { <---------------------------- here
        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);
    }

I need to think more of this, but this doesn't sound right at all.  There's
no such issue with precopy-only, and I'm surprised it is like that for years.

-- 
Peter Xu

Re: [PATCH 2/3] migration: Drop unnecessary check in ram's pending_exact()

[Peter Xu]

On Wed, Mar 20, 2024 at 03:46:44PM -0400, Peter Xu wrote:
> On Wed, Mar 20, 2024 at 08:21:30PM +0100, Nina Schoetterl-Glausch wrote:
> > On Wed, 2024-03-20 at 14:57 -0400, Peter Xu wrote:
> > > On Wed, Mar 20, 2024 at 06:51:26PM +0100, Nina Schoetterl-Glausch wrote:
> > > > On Wed, 2024-01-17 at 15:58 +0800, peterx@redhat.com wrote:
> > > > > From: Peter Xu <peterx@redhat.com>
> > > > > 
> > > > > When the migration frameworks fetches the exact pending sizes, it means
> > > > > this check:
> > > > > 
> > > > >   remaining_size < s->threshold_size
> > > > > 
> > > > > Must have been done already, actually at migration_iteration_run():
> > > > > 
> > > > >     if (must_precopy <= s->threshold_size) {
> > > > >         qemu_savevm_state_pending_exact(&must_precopy, &can_postcopy);
> > > > > 
> > > > > That should be after one round of ram_state_pending_estimate().  It makes
> > > > > the 2nd check meaningless and can be dropped.
> > > > > 
> > > > > To say it in another way, when reaching ->state_pending_exact(), we
> > > > > unconditionally sync dirty bits for precopy.
> > > > > 
> > > > > Then we can drop migrate_get_current() there too.
> > > > > 
> > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > 
> > > > Hi Peter,
> > > 
> > > Hi, Nina,
> > > 
> > > > 
> > > > could you have a look at this issue:
> > > > https://gitlab.com/qemu-project/qemu/-/issues/1565
> > > > 
> > > > which I reopened. Previous thread here:
> > > > 
> > > > https://lore.kernel.org/qemu-devel/20230324184129.3119575-1-nsg@linux.ibm.com/
> > > > 
> > > > I'm seeing migration failures with s390x TCG again, which look the same to me
> > > > as those a while back.
> > > 
> > > I'm still quite confused how that could be caused of this.
> > > 
> > > What you described in the previous bug report seems to imply some page was
> > > leftover in migration so some page got corrupted after migrated.
> > > 
> > > However what this patch mostly does is it can sync more than before even if
> > > I overlooked the condition check there (I still think the check is
> > > redundant, there's one outlier when remaining_size == threshold_size, but I
> > > don't think it should matter here as of now).  It'll make more sense if
> > > this patch made the sync less, but that's not the case but vice versa.
> > 
> > [...]
> > 
> > > In the previous discussion, you mentioned that you bisected to the commit
> > > and also verified the fix.  Now you also mentioned in the bz that you can't
> > > reporduce this bug manually.
> > > 
> > > Is it still possible to be reproduced with some scripts?  Do you also mean
> > > that it's harder to reproduce comparing to before?  In all cases, some way
> > > to reproduce it would definitely be helpful.
> > 
> > I tried running the kvm-unit-test a bunch of times in a loop and couldn't
> > trigger a failure. I just tried again on a different system and managed just
> > fine, yay. No idea why it wouldn't on the first system tho.
> 
> There's probably still a bug somewhere.  If reproduction rate changed, it's
> also a sign that it might not be directly relevant to this change, as
> otherwise it should reproduce the same as before.
> 
> > > 
> > > Even if we want to revert this change, we'll need to know whether this will
> > > fix your case so we need something to verify it before a revert.  I'll
> > > consider that the last though as I had a feeling this is papering over
> > > something else.
> > 
> > I can check if I can reproduce the issue before & after b0504edd ("migration:
> > Drop unnecessary check in ram's pending_exact()").
> > I can also check if I can reproduce it on x86, that worked last time.
> > Anything else? Ideas on how to pinpoint where the corruption happens?
> 
> I don't have a solid clue yet, but more information of the single case
> where it reproduced could help.
> 
> I saw from the bug link that the cmdline is pretty simple.  However still
> not sure of something that can be relevant.  E.g., did you use postcopy
> (including when postcopy-ram enabled but precopy completed)?  Is there any
> special device, like s390's CMMA (would that simplest cmdline include such
> a device; apologies, I have zero knowledge there before today)?
> 
> I _think_ when reading the code I already found something quite unusual,
> but only when postcopy is selected: I notice postcopy will frequently sync
> dirty bitmap while it doesn't really necessarily need to, because
> ram_state_pending_estimate() will report all ram as "can_postcopy"; it
> means it's highly likely that this check will 99.999% always be true simply
> because must_precopy can in most cases be zero:
> 
>     if (must_precopy <= s->threshold_size) { <---------------------------- here
>         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);
>     }
> 
> I need to think more of this, but this doesn't sound right at all.  There's
> no such issue with precopy-only, and I'm surprised it is like that for years.

It seems this can be a separate new bug.. possible introduced in the same
commit since 8.0.  I will post a patch for this soon.

One more thing to mention is I am aware Nicholas & Phil also hit some s390
tcg issues, and just recently there got a fix landed, I'm suspecting that
could also be relevant. See:

https://lore.kernel.org/qemu-devel/20240312201458.79532-1-philmd@linaro.org/
03bfc2188f physmem: Fix migration dirty bitmap coherency with TCG memory access

I would suspect this issue reproduces easier before this.  I think Nicholas
also mentioned there can be other bug floating around:

https://lore.kernel.org/qemu-devel/CZSDDVZW4G3L.6CV89ZRMQK9G@wheely/

Let me add all into this loop.

Thanks,

-- 
Peter Xu