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 <faro...@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
