| Message ID | 20190220115611.3192-6-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Make multifd not experimental | expand |
* Juan Quintela (quintela@redhat.com) wrote: > This way we can change the packet size in the future and everything > will work. We choose an arbitrary big number (100 times configured > size) as a limit about how big we will reallocate. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > migration/ram.c | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index e22d02760b..75a8fc21f8 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -723,13 +723,13 @@ static void multifd_pages_clear(MultiFDPages_t *pages) > static void multifd_send_fill_packet(MultiFDSendParams *p) > { > MultiFDPacket_t *packet = p->packet; > - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); > + uint32_t page_max = MULTIFD_PACKET_SIZE / qemu_target_page_size(); > int i; > > packet->magic = cpu_to_be32(MULTIFD_MAGIC); > packet->version = cpu_to_be32(MULTIFD_VERSION); > packet->flags = cpu_to_be32(p->flags); > - packet->pages_alloc = cpu_to_be32(page_count); > + packet->pages_alloc = cpu_to_be32(page_max); > packet->pages_used = cpu_to_be32(p->pages->used); > packet->next_packet_size = cpu_to_be32(p->next_packet_size); > packet->packet_num = cpu_to_be64(p->packet_num); > @@ -746,7 +746,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p) > static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) > { > MultiFDPacket_t *packet = p->packet; > - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); > + uint32_t pages_max = MULTIFD_PACKET_SIZE / qemu_target_page_size(); > RAMBlock *block; > int i; > > @@ -769,12 +769,24 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) > p->flags = be32_to_cpu(packet->flags); > > packet->pages_alloc = be32_to_cpu(packet->pages_alloc); > - if (packet->pages_alloc > page_count) { > + /* > + * If we recevied a packet that is 100 times bigger than expected > + * just stop migration. It is a magic number. > + */ > + if (packet->pages_alloc > pages_max * 100) { > error_setg(errp, "multifd: received packet " > - "with size %d and expected maximum size %d", > - packet->pages_alloc, page_count) ; > + "with size %d and expected size %d", > + packet->pages_alloc, pages_max) ; Should that end with pages_max * 100 ? > return -1; > } > + /* > + * We received a packet that is bigger than expected but inside > + * reasonable limits (see previous comment). Just reallocate. > + */ > + if (packet->pages_alloc > p->pages->allocated) { > + multifd_pages_clear(p->pages); > + multifd_pages_init(packet->pages_alloc); > + } > > p->pages->used = be32_to_cpu(packet->pages_used); > if (p->pages->used > packet->pages_alloc) { Other than that error message, I think it's OK, although the names get very confusing (max, alloc, allocated) Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > -- > 2.20.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> This way we can change the packet size in the future and everything >> will work. We choose an arbitrary big number (100 times configured >> size) as a limit about how big we will reallocate. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> migration/ram.c | 24 ++++++++++++++++++------ >> 1 file changed, 18 insertions(+), 6 deletions(-) >> >> diff --git a/migration/ram.c b/migration/ram.c >> index e22d02760b..75a8fc21f8 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -723,13 +723,13 @@ static void multifd_pages_clear(MultiFDPages_t *pages) >> static void multifd_send_fill_packet(MultiFDSendParams *p) >> { >> MultiFDPacket_t *packet = p->packet; >> - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); >> + uint32_t page_max = MULTIFD_PACKET_SIZE / qemu_target_page_size(); >> int i; >> >> packet->magic = cpu_to_be32(MULTIFD_MAGIC); >> packet->version = cpu_to_be32(MULTIFD_VERSION); >> packet->flags = cpu_to_be32(p->flags); >> - packet->pages_alloc = cpu_to_be32(page_count); >> + packet->pages_alloc = cpu_to_be32(page_max); >> packet->pages_used = cpu_to_be32(p->pages->used); >> packet->next_packet_size = cpu_to_be32(p->next_packet_size); >> packet->packet_num = cpu_to_be64(p->packet_num); >> @@ -746,7 +746,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p) >> static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) >> { >> MultiFDPacket_t *packet = p->packet; >> - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); >> + uint32_t pages_max = MULTIFD_PACKET_SIZE / qemu_target_page_size(); >> RAMBlock *block; >> int i; >> >> @@ -769,12 +769,24 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) >> p->flags = be32_to_cpu(packet->flags); >> >> packet->pages_alloc = be32_to_cpu(packet->pages_alloc); >> - if (packet->pages_alloc > page_count) { >> + /* >> + * If we recevied a packet that is 100 times bigger than expected >> + * just stop migration. It is a magic number. >> + */ >> + if (packet->pages_alloc > pages_max * 100) { >> error_setg(errp, "multifd: received packet " >> - "with size %d and expected maximum size %d", >> - packet->pages_alloc, page_count) ; >> + "with size %d and expected size %d", >> + packet->pages_alloc, pages_max) ; > > Should that end with pages_max * 100 ? Not sure. The *allocated* by defaault size is pages_max. If we receive bigger packets, we update it, but until a limit (arbitrary, I am open to other limits). So, what multifd is expecting here is pages_max. But it will cope with anything that is smaller than pages_max * 100. So, what I should put on the error message? 100 * pages_max or pages_max? It appears that for you it is simpler to understand pages_max * 100, and as I don't care, I am just changing it. >> return -1; >> } >> + /* >> + * We received a packet that is bigger than expected but inside >> + * reasonable limits (see previous comment). Just reallocate. >> + */ >> + if (packet->pages_alloc > p->pages->allocated) { >> + multifd_pages_clear(p->pages); >> + multifd_pages_init(packet->pages_alloc); >> + } >> >> p->pages->used = be32_to_cpu(packet->pages_used); >> if (p->pages->used > packet->pages_alloc) { > > Other than that error message, I think it's OK, although the names get > very confusing (max, alloc, allocated) I am open to suggestions. I just got out of names :-( > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Thanks.
diff --git a/migration/ram.c b/migration/ram.c index e22d02760b..75a8fc21f8 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -723,13 +723,13 @@ static void multifd_pages_clear(MultiFDPages_t *pages) static void multifd_send_fill_packet(MultiFDSendParams *p) { MultiFDPacket_t *packet = p->packet; - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); + uint32_t page_max = MULTIFD_PACKET_SIZE / qemu_target_page_size(); int i; packet->magic = cpu_to_be32(MULTIFD_MAGIC); packet->version = cpu_to_be32(MULTIFD_VERSION); packet->flags = cpu_to_be32(p->flags); - packet->pages_alloc = cpu_to_be32(page_count); + packet->pages_alloc = cpu_to_be32(page_max); packet->pages_used = cpu_to_be32(p->pages->used); packet->next_packet_size = cpu_to_be32(p->next_packet_size); packet->packet_num = cpu_to_be64(p->packet_num); @@ -746,7 +746,7 @@ static void multifd_send_fill_packet(MultiFDSendParams *p) static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) { MultiFDPacket_t *packet = p->packet; - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); + uint32_t pages_max = MULTIFD_PACKET_SIZE / qemu_target_page_size(); RAMBlock *block; int i; @@ -769,12 +769,24 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp) p->flags = be32_to_cpu(packet->flags); packet->pages_alloc = be32_to_cpu(packet->pages_alloc); - if (packet->pages_alloc > page_count) { + /* + * If we recevied a packet that is 100 times bigger than expected + * just stop migration. It is a magic number. + */ + if (packet->pages_alloc > pages_max * 100) { error_setg(errp, "multifd: received packet " - "with size %d and expected maximum size %d", - packet->pages_alloc, page_count) ; + "with size %d and expected size %d", + packet->pages_alloc, pages_max) ; return -1; } + /* + * We received a packet that is bigger than expected but inside + * reasonable limits (see previous comment). Just reallocate. + */ + if (packet->pages_alloc > p->pages->allocated) { + multifd_pages_clear(p->pages); + multifd_pages_init(packet->pages_alloc); + } p->pages->used = be32_to_cpu(packet->pages_used); if (p->pages->used > packet->pages_alloc) {
This way we can change the packet size in the future and everything will work. We choose an arbitrary big number (100 times configured size) as a limit about how big we will reallocate. Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/ram.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)