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