Re: [RFC PATCH v2 8/9] migration/multifd: Don't send ram data during SYNC

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Jul 22, 2024 at 02:59:13PM -0300, Fabiano Rosas wrote:
>> Skip saving and loading any ram data in the packet in the case of a
>> SYNC. This fixes a shortcoming of the current code which requires a
>> reset of the MultiFDPages_t fields right after the previous
>> pending_job finishes, otherwise the very next job might be a SYNC and
>> multifd_send_fill_packet() will put the stale values in the packet.
>> 
>> By not calling multifd_ram_fill_packet(), we can stop resetting
>> MultiFDPages_t in the multifd core and leave that to the client code.
>> 
>> Actually moving the reset function is not yet done because
>> pages->num==0 is used by the client code to determine whether the
>> MultiFDPages_t needs to be flushed. The subsequent patches will
>> replace that with a generic flag that is not dependent on
>> MultiFDPages_t.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/multifd.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>> 
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index d25b8658b2..4394ca6ade 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -438,6 +438,7 @@ void multifd_send_fill_packet(MultiFDSendParams *p)
>>  {
>>      MultiFDPacket_t *packet = p->packet;
>>      uint64_t packet_num;
>> +    bool sync_packet = p->flags & MULTIFD_FLAG_SYNC;
>>  
>>      memset(packet, 0, p->packet_len);
>>  
>> @@ -452,7 +453,9 @@ void multifd_send_fill_packet(MultiFDSendParams *p)
>>  
>>      p->packets_sent++;
>>  
>> -    multifd_ram_fill_packet(p);
>> +    if (!sync_packet) {
>> +        multifd_ram_fill_packet(p);
>> +    }
>>  
>>      trace_multifd_send(p->id, packet_num,
>>                         be32_to_cpu(packet->normal_pages),
>> @@ -563,7 +566,12 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams 
>> *p, Error **errp)
>>      p->packet_num = be64_to_cpu(packet->packet_num);
>>      p->packets_recved++;
>>  
>> -    ret = multifd_ram_unfill_packet(p, errp);
>> +    if (p->flags & MULTIFD_FLAG_SYNC) {
>> +        p->normal_num = 0;
>> +        p->zero_num = 0;
>
> Instead of this, I wonder whether we shouldn't touch those fields at all,
> but:
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 0a85951d58..55abd9a1ef 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -1547,7 +1547,9 @@ static void *multifd_recv_thread(void *opaque)
>              flags = p->flags;
>              /* recv methods don't know how to handle the SYNC flag */
>              p->flags &= ~MULTIFD_FLAG_SYNC;
> -            has_data = p->normal_num || p->zero_num;
> +
> +            if (!(flags & MULTIFD_FLAG_SYNC))
> +                has_data = p->normal_num || p->zero_num;
>              qemu_mutex_unlock(&p->mutex);
>          } else {
>              /*

Good idea.

>
>> +    } else {
>> +        ret = multifd_ram_unfill_packet(p, errp);
>> +    }
>>  
>>      trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->zero_num,
>>                         p->flags, p->next_packet_size);
>> -- 
>> 2.35.3
>>