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

[Peter Xu <peterx@redhat.com>]

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