[PATCH 0/2] migration: A couple of cleanups

[PATCH 0/2] migration: A couple of cleanups

[Fabiano Rosas]

Postcopy code was moved and some if postcopy were left behind.

Multifd has an accounting issue in tracepoints.

Fabiano Rosas (2):
  migration/multifd: Fix received packets tracepoint
  migration: Trivial cleanups for postcopy

 migration/migration.c  | 28 ++++++++++------------------
 migration/multifd.c    |  6 +-----
 migration/trace-events |  4 ++--
 3 files changed, 13 insertions(+), 25 deletions(-)

-- 
2.35.3

[PATCH 1/2] migration/multifd: Fix received packets tracepoint

[Fabiano Rosas]

When qatomic_fetch_inc() started being used to count the number of
packets sent, the printing of the number of packets received stopped
matching the number of packets sent.

Fix by moving the increment of the number of packets on the recv side
to multifd_recv_unfill_packet().

Also change the tracepoint text because "packet num" is ambiguous for
the sync since the packet number of the actual sync packet will be one
less than the total number of packets seen so far.

Fixes: 98ea497d8b ("migration/multifd: Fix MultiFDSendParams.packet_num race")
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c    | 6 +-----
 migration/trace-events | 4 ++--
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index dfb5189f0e..1a16155864 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -310,6 +310,7 @@ static int multifd_recv_unfill_packet_ram(MultiFDRecvParams *p, Error **errp)
 
 static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
 {
+    qatomic_inc(&multifd_recv_state->packet_num);
     p->packets_recved++;
 
     if (p->flags & MULTIFD_FLAG_DEVICE_STATE) {
@@ -1222,11 +1223,6 @@ void multifd_recv_sync_main(void)
     for (i = 0; i < thread_count; i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
 
-        WITH_QEMU_LOCK_GUARD(&p->mutex) {
-            if (multifd_recv_state->packet_num < p->packet_num) {
-                multifd_recv_state->packet_num = p->packet_num;
-            }
-        }
         trace_multifd_recv_sync_main_signal(p->id);
         qemu_sem_post(&p->sem_sync);
     }
diff --git a/migration/trace-events b/migration/trace-events
index c506e11a2e..48acb126f5 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -133,7 +133,7 @@ multifd_new_send_channel_async(uint8_t id) "channel %u"
 multifd_new_send_channel_async_error(uint8_t id, void *err) "channel=%u err=%p"
 multifd_recv_unfill(uint8_t id, uint64_t packet_num, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " flags 0x%x next packet size %u"
 multifd_recv_new_channel(uint8_t id) "channel %u"
-multifd_recv_sync_main(long packet_num) "packet num %ld"
+multifd_recv_sync_main(long packet_num) "packets before sync %ld"
 multifd_recv_sync_main_signal(uint8_t id) "channel %u"
 multifd_recv_sync_main_wait(uint8_t id) "iter %u"
 multifd_recv_terminate_threads(bool error) "error %d"
@@ -142,7 +142,7 @@ multifd_recv_thread_start(uint8_t id) "%u"
 multifd_send_fill(uint8_t id, uint64_t packet_num, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " flags 0x%x next packet size %u"
 multifd_send_ram_fill(uint8_t id, uint32_t normal, uint32_t zero) "channel %u normal pages %u zero pages %u"
 multifd_send_error(uint8_t id) "channel %u"
-multifd_send_sync_main(long packet_num) "packet num %ld"
+multifd_send_sync_main(long packet_num) "packets before sync %ld"
 multifd_send_sync_main_signal(uint8_t id) "channel %u"
 multifd_send_sync_main_wait(uint8_t id) "channel %u"
 multifd_send_terminate_threads(void) ""
-- 
2.35.3

Re: [PATCH 1/2] migration/multifd: Fix received packets tracepoint

[Peter Xu]

On Wed, Apr 16, 2025 at 10:43:55AM -0300, Fabiano Rosas wrote:
> When qatomic_fetch_inc() started being used to count the number of
> packets sent, the printing of the number of packets received stopped
> matching the number of packets sent.
> 
> Fix by moving the increment of the number of packets on the recv side
> to multifd_recv_unfill_packet().
> 
> Also change the tracepoint text because "packet num" is ambiguous for
> the sync since the packet number of the actual sync packet will be one
> less than the total number of packets seen so far.

Would this be a hint that the recv side may not really need a global
packet_num counter, at all?

On source, it needs to be there if we want to have an unified unique ID for
each multifd packet, so that when allcating a packet we need them to be
assigned properly.

On dest, it almost only receives packets, it's still unclear to me how the
recv packet_num global var could help us considering we have the per-packet
trace in trace_multifd_recv_unfill() dumping the unique ID for each..

So.. would it be of any use?  Would it be better if we remove it instead?

> 
> Fixes: 98ea497d8b ("migration/multifd: Fix MultiFDSendParams.packet_num race")
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/multifd.c    | 6 +-----
>  migration/trace-events | 4 ++--
>  2 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index dfb5189f0e..1a16155864 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -310,6 +310,7 @@ static int multifd_recv_unfill_packet_ram(MultiFDRecvParams *p, Error **errp)
>  
>  static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>  {
> +    qatomic_inc(&multifd_recv_state->packet_num);
>      p->packets_recved++;
>  
>      if (p->flags & MULTIFD_FLAG_DEVICE_STATE) {
> @@ -1222,11 +1223,6 @@ void multifd_recv_sync_main(void)
>      for (i = 0; i < thread_count; i++) {
>          MultiFDRecvParams *p = &multifd_recv_state->params[i];
>  
> -        WITH_QEMU_LOCK_GUARD(&p->mutex) {
> -            if (multifd_recv_state->packet_num < p->packet_num) {
> -                multifd_recv_state->packet_num = p->packet_num;
> -            }
> -        }
>          trace_multifd_recv_sync_main_signal(p->id);
>          qemu_sem_post(&p->sem_sync);
>      }
> diff --git a/migration/trace-events b/migration/trace-events
> index c506e11a2e..48acb126f5 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -133,7 +133,7 @@ multifd_new_send_channel_async(uint8_t id) "channel %u"
>  multifd_new_send_channel_async_error(uint8_t id, void *err) "channel=%u err=%p"
>  multifd_recv_unfill(uint8_t id, uint64_t packet_num, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " flags 0x%x next packet size %u"
>  multifd_recv_new_channel(uint8_t id) "channel %u"
> -multifd_recv_sync_main(long packet_num) "packet num %ld"
> +multifd_recv_sync_main(long packet_num) "packets before sync %ld"
>  multifd_recv_sync_main_signal(uint8_t id) "channel %u"
>  multifd_recv_sync_main_wait(uint8_t id) "iter %u"
>  multifd_recv_terminate_threads(bool error) "error %d"
> @@ -142,7 +142,7 @@ multifd_recv_thread_start(uint8_t id) "%u"
>  multifd_send_fill(uint8_t id, uint64_t packet_num, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " flags 0x%x next packet size %u"
>  multifd_send_ram_fill(uint8_t id, uint32_t normal, uint32_t zero) "channel %u normal pages %u zero pages %u"
>  multifd_send_error(uint8_t id) "channel %u"
> -multifd_send_sync_main(long packet_num) "packet num %ld"
> +multifd_send_sync_main(long packet_num) "packets before sync %ld"
>  multifd_send_sync_main_signal(uint8_t id) "channel %u"
>  multifd_send_sync_main_wait(uint8_t id) "channel %u"
>  multifd_send_terminate_threads(void) ""
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH 1/2] migration/multifd: Fix received packets tracepoint

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Apr 16, 2025 at 10:43:55AM -0300, Fabiano Rosas wrote:
>> When qatomic_fetch_inc() started being used to count the number of
>> packets sent, the printing of the number of packets received stopped
>> matching the number of packets sent.
>> 
>> Fix by moving the increment of the number of packets on the recv side
>> to multifd_recv_unfill_packet().
>> 
>> Also change the tracepoint text because "packet num" is ambiguous for
>> the sync since the packet number of the actual sync packet will be one
>> less than the total number of packets seen so far.
>
> Would this be a hint that the recv side may not really need a global
> packet_num counter, at all?
>
> On source, it needs to be there if we want to have an unified unique ID for
> each multifd packet, so that when allcating a packet we need them to be
> assigned properly.
>
> On dest, it almost only receives packets, it's still unclear to me how the
> recv packet_num global var could help us considering we have the per-packet
> trace in trace_multifd_recv_unfill() dumping the unique ID for each..
>
> So.. would it be of any use?  Would it be better if we remove it instead?
>

It's good for debugging. The p->packet_num on the recv side will at some
point contain the total number of packets sent, but it's hard to answer
how many packets have arrived at any given moment just by looking at
trace_multifd_recv_unfill(). Packets could arrive out of order.

I'm inclined to say that p->packet_num is the one that's useless. After
this patch, it's only there to hold an endianness swap. We can reach the
BE value via p->packet->packet_num already.

>> 
>> Fixes: 98ea497d8b ("migration/multifd: Fix MultiFDSendParams.packet_num race")
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/multifd.c    | 6 +-----
>>  migration/trace-events | 4 ++--
>>  2 files changed, 3 insertions(+), 7 deletions(-)
>> 
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index dfb5189f0e..1a16155864 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -310,6 +310,7 @@ static int multifd_recv_unfill_packet_ram(MultiFDRecvParams *p, Error **errp)
>>  
>>  static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>>  {
>> +    qatomic_inc(&multifd_recv_state->packet_num);
>>      p->packets_recved++;
>>  
>>      if (p->flags & MULTIFD_FLAG_DEVICE_STATE) {
>> @@ -1222,11 +1223,6 @@ void multifd_recv_sync_main(void)
>>      for (i = 0; i < thread_count; i++) {
>>          MultiFDRecvParams *p = &multifd_recv_state->params[i];
>>  
>> -        WITH_QEMU_LOCK_GUARD(&p->mutex) {
>> -            if (multifd_recv_state->packet_num < p->packet_num) {
>> -                multifd_recv_state->packet_num = p->packet_num;
>> -            }
>> -        }
>>          trace_multifd_recv_sync_main_signal(p->id);
>>          qemu_sem_post(&p->sem_sync);
>>      }
>> diff --git a/migration/trace-events b/migration/trace-events
>> index c506e11a2e..48acb126f5 100644
>> --- a/migration/trace-events
>> +++ b/migration/trace-events
>> @@ -133,7 +133,7 @@ multifd_new_send_channel_async(uint8_t id) "channel %u"
>>  multifd_new_send_channel_async_error(uint8_t id, void *err) "channel=%u err=%p"
>>  multifd_recv_unfill(uint8_t id, uint64_t packet_num, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " flags 0x%x next packet size %u"
>>  multifd_recv_new_channel(uint8_t id) "channel %u"
>> -multifd_recv_sync_main(long packet_num) "packet num %ld"
>> +multifd_recv_sync_main(long packet_num) "packets before sync %ld"
>>  multifd_recv_sync_main_signal(uint8_t id) "channel %u"
>>  multifd_recv_sync_main_wait(uint8_t id) "iter %u"
>>  multifd_recv_terminate_threads(bool error) "error %d"
>> @@ -142,7 +142,7 @@ multifd_recv_thread_start(uint8_t id) "%u"
>>  multifd_send_fill(uint8_t id, uint64_t packet_num, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " flags 0x%x next packet size %u"
>>  multifd_send_ram_fill(uint8_t id, uint32_t normal, uint32_t zero) "channel %u normal pages %u zero pages %u"
>>  multifd_send_error(uint8_t id) "channel %u"
>> -multifd_send_sync_main(long packet_num) "packet num %ld"
>> +multifd_send_sync_main(long packet_num) "packets before sync %ld"
>>  multifd_send_sync_main_signal(uint8_t id) "channel %u"
>>  multifd_send_sync_main_wait(uint8_t id) "channel %u"
>>  multifd_send_terminate_threads(void) ""
>> -- 
>> 2.35.3
>> 

Re: [PATCH 1/2] migration/multifd: Fix received packets tracepoint

[Peter Xu]

On Mon, May 05, 2025 at 06:51:58PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Wed, Apr 16, 2025 at 10:43:55AM -0300, Fabiano Rosas wrote:
> >> When qatomic_fetch_inc() started being used to count the number of
> >> packets sent, the printing of the number of packets received stopped
> >> matching the number of packets sent.
> >> 
> >> Fix by moving the increment of the number of packets on the recv side
> >> to multifd_recv_unfill_packet().
> >> 
> >> Also change the tracepoint text because "packet num" is ambiguous for
> >> the sync since the packet number of the actual sync packet will be one
> >> less than the total number of packets seen so far.
> >
> > Would this be a hint that the recv side may not really need a global
> > packet_num counter, at all?
> >
> > On source, it needs to be there if we want to have an unified unique ID for
> > each multifd packet, so that when allcating a packet we need them to be
> > assigned properly.
> >
> > On dest, it almost only receives packets, it's still unclear to me how the
> > recv packet_num global var could help us considering we have the per-packet
> > trace in trace_multifd_recv_unfill() dumping the unique ID for each..
> >
> > So.. would it be of any use?  Would it be better if we remove it instead?
> >
> 
> It's good for debugging. The p->packet_num on the recv side will at some
> point contain the total number of packets sent, but it's hard to answer
> how many packets have arrived at any given moment just by looking at
> trace_multifd_recv_unfill(). Packets could arrive out of order.

IMHO it can also be confusing at the same time..

E.g., before this patch the value doesn't mean all the packets smaller than
that have landed.. but only the max of whatever recv side got.

While after this patch, IIUC it will increase each time, but it can be
confusing in another way when e.g. the packets arrives in different order
and this value can imply something weird.  Consider below seq:

  Packets arrival: 1,2,3,5

Then this var will be 4 even after 5 received, but without getting 4..

But yeah maybe it's still helpful when we only want to sanity check the
total amount matches on src.  I'm OK we keep it like the new definition if
that helps in any way.

> 
> I'm inclined to say that p->packet_num is the one that's useless. After
> this patch, it's only there to hold an endianness swap. We can reach the
> BE value via p->packet->packet_num already.

Yep, I think the tracepoint is more important than the value cached,
especially when cached twice..

-- 
Peter Xu

[PATCH 2/2] migration: Trivial cleanups for postcopy

[Fabiano Rosas]

Some general cleanups of silly things that were left behind when
refactoring code.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index d46e776e24..89b1de0ab5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2732,19 +2732,15 @@ static int postcopy_start(MigrationState *ms, Error **errp)
     }
 
     /*
-     * in Finish migrate and with the io-lock held everything should
+     * in FINISH_MIGRATE and with the BQL held everything should
      * be quiet, but we've potentially still got dirty pages and we
      * need to tell the destination to throw any pages it's already received
      * that are dirty
      */
-    if (migrate_postcopy_ram()) {
-        ram_postcopy_send_discard_bitmap(ms);
-    }
+    ram_postcopy_send_discard_bitmap(ms);
 
-    if (migrate_postcopy_ram()) {
-        /* Ping just for debugging, helps line traces up */
-        qemu_savevm_send_ping(ms->to_dst_file, 2);
-    }
+    /* Ping just for debugging, helps line traces up */
+    qemu_savevm_send_ping(ms->to_dst_file, 2);
 
     /*
      * While loading the device state we may trigger page transfer
@@ -2774,9 +2770,7 @@ static int postcopy_start(MigrationState *ms, Error **errp)
         goto fail_closefb;
     }
 
-    if (migrate_postcopy_ram()) {
-        qemu_savevm_send_ping(fb, 3);
-    }
+    qemu_savevm_send_ping(fb, 3);
 
     qemu_savevm_send_postcopy_run(fb);
 
@@ -2807,13 +2801,11 @@ static int postcopy_start(MigrationState *ms, Error **errp)
 
     migration_downtime_end(ms);
 
-    if (migrate_postcopy_ram()) {
-        /*
-         * Although this ping is just for debug, it could potentially be
-         * used for getting a better measurement of downtime at the source.
-         */
-        qemu_savevm_send_ping(ms->to_dst_file, 4);
-    }
+    /*
+     * Although this ping is just for debug, it could potentially be
+     * used for getting a better measurement of downtime at the source.
+     */
+    qemu_savevm_send_ping(ms->to_dst_file, 4);
 
     if (migrate_release_ram()) {
         ram_postcopy_migrated_memory_release(ms);
-- 
2.35.3

Re: [PATCH 2/2] migration: Trivial cleanups for postcopy

[Peter Xu]

On Wed, Apr 16, 2025 at 10:43:56AM -0300, Fabiano Rosas wrote:
> Some general cleanups of silly things that were left behind when
> refactoring code.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index d46e776e24..89b1de0ab5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2732,19 +2732,15 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>      }
>  
>      /*
> -     * in Finish migrate and with the io-lock held everything should
> +     * in FINISH_MIGRATE and with the BQL held everything should
>       * be quiet, but we've potentially still got dirty pages and we
>       * need to tell the destination to throw any pages it's already received
>       * that are dirty
>       */
> -    if (migrate_postcopy_ram()) {
> -        ram_postcopy_send_discard_bitmap(ms);
> -    }
> +    ram_postcopy_send_discard_bitmap(ms);

IIUC it was from:

commit 58110f0acb1a33e2bc60a2f1b26d2690a92e8a14
Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Date:   Mon Jul 10 19:30:16 2017 +0300

    migration: split common postcopy out of ram postcopy

I need to confess I know nothing about migrate_dirty_bitmaps().. and I
still think the name is not well chosen - migration definitely has its own
dirty bitmap, and it has nothing to do with block layer..

Obviously, it was for postcopy support of block layer stuff, but I don't
know who is using it, how, and when..

Is people using this feature?  Maybe we should have an unit test, and a
doc?  Copying Vladimir.

>  
> -    if (migrate_postcopy_ram()) {
> -        /* Ping just for debugging, helps line traces up */
> -        qemu_savevm_send_ping(ms->to_dst_file, 2);
> -    }
> +    /* Ping just for debugging, helps line traces up */
> +    qemu_savevm_send_ping(ms->to_dst_file, 2);
>  
>      /*
>       * While loading the device state we may trigger page transfer
> @@ -2774,9 +2770,7 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>          goto fail_closefb;
>      }
>  
> -    if (migrate_postcopy_ram()) {
> -        qemu_savevm_send_ping(fb, 3);
> -    }
> +    qemu_savevm_send_ping(fb, 3);
>  
>      qemu_savevm_send_postcopy_run(fb);
>  
> @@ -2807,13 +2801,11 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>  
>      migration_downtime_end(ms);
>  
> -    if (migrate_postcopy_ram()) {
> -        /*
> -         * Although this ping is just for debug, it could potentially be
> -         * used for getting a better measurement of downtime at the source.
> -         */
> -        qemu_savevm_send_ping(ms->to_dst_file, 4);
> -    }
> +    /*
> +     * Although this ping is just for debug, it could potentially be
> +     * used for getting a better measurement of downtime at the source.
> +     */
> +    qemu_savevm_send_ping(ms->to_dst_file, 4);
>  
>      if (migrate_release_ram()) {
>          ram_postcopy_migrated_memory_release(ms);
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH 0/2] migration: A couple of cleanups

[Juraj Marcin]

Hi Fabiano

On 2025-04-16 10:43, Fabiano Rosas wrote:
> Postcopy code was moved and some if postcopy were left behind.
> 
> Multifd has an accounting issue in tracepoints.
> 
> Fabiano Rosas (2):
>   migration/multifd: Fix received packets tracepoint
>   migration: Trivial cleanups for postcopy
> 
>  migration/migration.c  | 28 ++++++++++------------------
>  migration/multifd.c    |  6 +-----
>  migration/trace-events |  4 ++--
>  3 files changed, 13 insertions(+), 25 deletions(-)
> 
> -- 
> 2.35.3
> 

Both patches look good to me.

Reviewed-by: Juraj Marcin <jmarcin@redhat.com>