Peter Xu <pet...@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 <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
>>
