| Message ID | 20240823173911.6712-11-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration/multifd: Remove multifd_send_state->pages | expand |
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; > +}
On 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>
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; >> +}
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"
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(-)