Re: [RFC PATCH v2 7/9] migration/multifd: Isolate ram pages packet data

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Jul 22, 2024 at 02:59:12PM -0300, Fabiano Rosas wrote:
>> While we cannot yet disentangle the multifd packet from page data, we
>> can make the code a bit cleaner by setting the page-related fields in
>> a separate function.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/multifd.c | 97 ++++++++++++++++++++++++++++-----------------
>>  1 file changed, 61 insertions(+), 36 deletions(-)
>> 
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index fcdb12e04f..d25b8658b2 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -409,65 +409,62 @@ static int multifd_recv_initial_packet(QIOChannel *c, Error **errp)
>>      return msg.id;
>>  }
>>  
>> -void multifd_send_fill_packet(MultiFDSendParams *p)
>> +static void multifd_ram_fill_packet(MultiFDSendParams *p)
>>  {
>>      MultiFDPacket_t *packet = p->packet;
>>      MultiFDPages_t *pages = &p->data->u.ram;
>> -    uint64_t packet_num;
>>      uint32_t zero_num = pages->num - pages->normal_num;
>> -    int i;
>>  
>> -    packet->flags = cpu_to_be32(p->flags);
>>      packet->pages_alloc = cpu_to_be32(pages->allocated);
>>      packet->normal_pages = cpu_to_be32(pages->normal_num);
>>      packet->zero_pages = cpu_to_be32(zero_num);
>> -    packet->next_packet_size = cpu_to_be32(p->next_packet_size);
>
> Definitely good intention, but I had a feeling that this will need to be
> reorganized again when Maciej reworks on top, due to the fact that
> next_packet_size will be ram-private field, simply because it's defined
> after pages_alloc and normal_pages...
>
> E.g., see:
>
> https://lore.kernel.org/r/41dedaf2c9abebb5e45f88c052daa26320715a92.1718717584.git.maciej.szmigiero@oracle.com
>
> Where the new MultiFDPacketHdr_t cannot include next_packet_size (even if
> VFIO will need that too..).

Isn't it just a matter of setting next_packet_size in the other path as
well?

@Maciej, can you comment?

>
> typedef struct {
>     uint32_t magic;
>     uint32_t version;
>     uint32_t flags;
> } __attribute__((packed)) MultiFDPacketHdr_t;
>
> So _maybe_ it's easier we drop this patch and leave that part to Maciej to
> identify which is common and which is arm/vfio specific.  No strong
> opinions here.
>

I could drop it if that's preferrable. However, patch 8/9 is absolutely
necessary so we can remove this madness of having to clear
MultiFDPages_t fields at the multifd_send_thread() top level. It took me
a whole day to figure that one out and that bug has been manifesting
ever since I started working on multifd.

I'm not sure how we'll do that without this patch. Maybe it's better I
fix in this one whatever you guys think needs fixing.

>> -
>> -    packet_num = qatomic_fetch_inc(&multifd_send_state->packet_num);
>> -    packet->packet_num = cpu_to_be64(packet_num);
>>  
>>      if (pages->block) {
>>          strncpy(packet->ramblock, pages->block->idstr, 256);
>>      }
>>  
>> -    for (i = 0; i < pages->num; i++) {
>> +    for (int i = 0; i < pages->num; i++) {
>>          /* there are architectures where ram_addr_t is 32 bit */
>>          uint64_t temp = pages->offset[i];
>>  
>>          packet->offset[i] = cpu_to_be64(temp);
>>      }
>>  
>> -    p->packets_sent++;
>>      p->total_normal_pages += pages->normal_num;
>>      p->total_zero_pages += zero_num;
>> +}
>>  
>> -    trace_multifd_send(p->id, packet_num, pages->normal_num, zero_num,
>> +void multifd_send_fill_packet(MultiFDSendParams *p)
>> +{
>> +    MultiFDPacket_t *packet = p->packet;
>> +    uint64_t packet_num;
>> +
>> +    memset(packet, 0, p->packet_len);
>> +
>> +    packet->magic = cpu_to_be32(MULTIFD_MAGIC);
>> +    packet->version = cpu_to_be32(MULTIFD_VERSION);
>> +
>> +    packet->flags = cpu_to_be32(p->flags);
>> +    packet->next_packet_size = cpu_to_be32(p->next_packet_size);
>> +
>> +    packet_num = qatomic_fetch_inc(&multifd_send_state->packet_num);
>> +    packet->packet_num = cpu_to_be64(packet_num);
>> +
>> +    p->packets_sent++;
>> +
>> +    multifd_ram_fill_packet(p);
>> +
>> +    trace_multifd_send(p->id, packet_num,
>> +                       be32_to_cpu(packet->normal_pages),
>> +                       be32_to_cpu(packet->zero_pages),
>>                         p->flags, p->next_packet_size);
>>  }
>>  
>> -static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>> +static int multifd_ram_unfill_packet(MultiFDRecvParams *p, Error **errp)
>>  {
>>      MultiFDPacket_t *packet = p->packet;
>>      int i;
>>  
>> -    packet->magic = be32_to_cpu(packet->magic);
>> -    if (packet->magic != MULTIFD_MAGIC) {
>> -        error_setg(errp, "multifd: received packet "
>> -                   "magic %x and expected magic %x",
>> -                   packet->magic, MULTIFD_MAGIC);
>> -        return -1;
>> -    }
>> -
>> -    packet->version = be32_to_cpu(packet->version);
>> -    if (packet->version != MULTIFD_VERSION) {
>> -        error_setg(errp, "multifd: received packet "
>> -                   "version %u and expected version %u",
>> -                   packet->version, MULTIFD_VERSION);
>> -        return -1;
>> -    }
>> -
>> -    p->flags = be32_to_cpu(packet->flags);
>> -
>>      packet->pages_alloc = be32_to_cpu(packet->pages_alloc);
>>      /*
>>       * If we received a packet that is 100 times bigger than expected
>> @@ -496,15 +493,9 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>>          return -1;
>>      }
>>  
>> -    p->next_packet_size = be32_to_cpu(packet->next_packet_size);
>> -    p->packet_num = be64_to_cpu(packet->packet_num);
>> -    p->packets_recved++;
>>      p->total_normal_pages += p->normal_num;
>>      p->total_zero_pages += p->zero_num;
>>  
>> -    trace_multifd_recv(p->id, p->packet_num, p->normal_num, p->zero_num,
>> -                       p->flags, p->next_packet_size);
>> -
>>      if (p->normal_num == 0 && p->zero_num == 0) {
>>          return 0;
>>      }
>> @@ -546,6 +537,40 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>>      return 0;
>>  }
>>  
>> +static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>> +{
>> +    MultiFDPacket_t *packet = p->packet;
>> +    int ret = 0;
>> +
>> +    packet->magic = be32_to_cpu(packet->magic);
>> +    if (packet->magic != MULTIFD_MAGIC) {
>> +        error_setg(errp, "multifd: received packet "
>> +                   "magic %x and expected magic %x",
>> +                   packet->magic, MULTIFD_MAGIC);
>> +        return -1;
>> +    }
>> +
>> +    packet->version = be32_to_cpu(packet->version);
>> +    if (packet->version != MULTIFD_VERSION) {
>> +        error_setg(errp, "multifd: received packet "
>> +                   "version %u and expected version %u",
>> +                   packet->version, MULTIFD_VERSION);
>> +        return -1;
>> +    }
>> +
>> +    p->flags = be32_to_cpu(packet->flags);
>> +    p->next_packet_size = be32_to_cpu(packet->next_packet_size);
>> +    p->packet_num = be64_to_cpu(packet->packet_num);
>> +    p->packets_recved++;
>> +
>> +    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);
>> +
>> +    return ret;
>> +}
>> +
>>  static bool multifd_send_should_exit(void)
>>  {
>>      return qatomic_read(&multifd_send_state->exiting);
>> -- 
>> 2.35.3
>>