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 | 99 +++++++++++++++++++++++++-----------------
migration/trace-events | 5 ++-
2 files changed, 63 insertions(+), 41 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index df8dfcc98f..d64fcdf4ac 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -424,65 +424,61 @@ 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(multifd_ram_page_count());
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);
-
- 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);
}
+ trace_multifd_send_ram_fill(p->id, 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++;
- trace_multifd_send(p->id, packet_num, pages->normal_num, zero_num,
- p->flags, p->next_packet_size);
+ multifd_ram_fill_packet(p);
+
+ trace_multifd_send_fill(p->id, packet_num,
+ 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;
uint32_t page_count = multifd_ram_page_count();
uint32_t page_size = multifd_ram_page_size();
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
@@ -511,13 +507,6 @@ 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++;
-
- 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;
}
@@ -559,6 +548,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_unfill(p->id, p->packet_num, p->flags,
+ p->next_packet_size);
+
+ return ret;
+}
+
static bool multifd_send_should_exit(void)
{
return qatomic_read(&multifd_send_state->exiting);
@@ -1203,8 +1226,6 @@ bool multifd_send_setup(void)
p->packet_len = sizeof(MultiFDPacket_t)
+ sizeof(uint64_t) * page_count;
p->packet = g_malloc0(p->packet_len);
- p->packet->magic = cpu_to_be32(MULTIFD_MAGIC);
- p->packet->version = cpu_to_be32(MULTIFD_VERSION);
}
p->name = g_strdup_printf("mig/src/send_%d", i);
p->write_flags = 0;
diff --git a/migration/trace-events b/migration/trace-events
index 0887cef912..c65902f042 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -128,7 +128,7 @@ postcopy_preempt_reset_channel(void) ""
# multifd.c
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(uint8_t id, uint64_t packet_num, uint32_t normal, uint32_t zero, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " normal pages %u zero pages %u flags 0x%x next packet size %u"
+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_signal(uint8_t id) "channel %u"
@@ -136,7 +136,8 @@ multifd_recv_sync_main_wait(uint8_t id) "iter %u"
multifd_recv_terminate_threads(bool error) "error %d"
multifd_recv_thread_end(uint8_t id, uint64_t packets) "channel %u packets %" PRIu64
multifd_recv_thread_start(uint8_t id) "%u"
-multifd_send(uint8_t id, uint64_t packet_num, uint32_t normal_pages, uint32_t zero_pages, uint32_t flags, uint32_t next_packet_size) "channel %u packet_num %" PRIu64 " normal pages %u zero pages %u flags 0x%x next packet size %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_signal(uint8_t id) "channel %u"
--
2.35.3On Fri, Aug 23, 2024 at 02:39:05PM -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> Reviewed-by: Peter Xu <peterx@redhat.com> -- Peter Xu
On 23/8/24 19:39, 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 | 99 +++++++++++++++++++++++++-----------------
> migration/trace-events | 5 ++-
> 2 files changed, 63 insertions(+), 41 deletions(-)
> -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;
> uint32_t page_count = multifd_ram_page_count();
> uint32_t page_size = multifd_ram_page_size();
> 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
> @@ -511,13 +507,6 @@ 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++;
> -
> - 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;
> }
> @@ -559,6 +548,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);
Pre-existing but since you modify this code, maybe cleaner to
"unfill" all packet fields, then check for sane magic/version
and return error, to not let a packet half-swapped. Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> +
> + trace_multifd_recv_unfill(p->id, p->packet_num, p->flags,
> + p->next_packet_size);
> +
> + return ret;
> +}
Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> On 23/8/24 19:39, 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 | 99 +++++++++++++++++++++++++-----------------
>> migration/trace-events | 5 ++-
>> 2 files changed, 63 insertions(+), 41 deletions(-)
>
>
>> -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;
>> uint32_t page_count = multifd_ram_page_count();
>> uint32_t page_size = multifd_ram_page_size();
>> 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
>> @@ -511,13 +507,6 @@ 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++;
>> -
>> - 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;
>> }
>> @@ -559,6 +548,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);
>
> Pre-existing but since you modify this code, maybe cleaner to
> "unfill" all packet fields, then check for sane magic/version
> and return error, to not let a packet half-swapped. Otherwise:
I don't even see the point of byte-swapping the packet
in-place. Probably better to have all the swaps into the variables in
the unfill function and leave the packet unchanged. I'll add another
patch to the series.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>> +
>> + trace_multifd_recv_unfill(p->id, p->packet_num, p->flags,
>> + p->next_packet_size);
>> +
>> + return ret;
>> +}
© 2016 - 2024 Red Hat, Inc.