| Message ID | 20211124100617.19786-2-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Migration: Transmit and detect zero pages in the multifd threads | expand |
* Juan Quintela (quintela@redhat.com) wrote: > We are divining by page_size to multiply again in the only use. ^--- typo > Once there, impreve the comments. ^--- typo > > Signed-off-by: Juan Quintela <quintela@redhat.com> OK, with the typo's fixed: Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> but, could you also explain the x 2 (that's no worse than the current code); is this defined somewhere in zlib? I thought there was a routine that told you the worst case? Dave > --- > migration/multifd-zlib.c | 13 ++++--------- > migration/multifd-zstd.c | 13 ++++--------- > 2 files changed, 8 insertions(+), 18 deletions(-) > > diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c > index ab4ba75d75..3fc7813b44 100644 > --- a/migration/multifd-zlib.c > +++ b/migration/multifd-zlib.c > @@ -42,7 +42,6 @@ struct zlib_data { > */ > static int zlib_send_setup(MultiFDSendParams *p, Error **errp) > { > - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); > struct zlib_data *z = g_malloc0(sizeof(struct zlib_data)); > z_stream *zs = &z->zs; > > @@ -54,9 +53,8 @@ static int zlib_send_setup(MultiFDSendParams *p, Error **errp) > error_setg(errp, "multifd %d: deflate init failed", p->id); > return -1; > } > - /* We will never have more than page_count pages */ > - z->zbuff_len = page_count * qemu_target_page_size(); > - z->zbuff_len *= 2; > + /* To be safe, we reserve twice the size of the packet */ > + z->zbuff_len = MULTIFD_PACKET_SIZE * 2; > z->zbuff = g_try_malloc(z->zbuff_len); > if (!z->zbuff) { > deflateEnd(&z->zs); > @@ -180,7 +178,6 @@ static int zlib_send_write(MultiFDSendParams *p, uint32_t used, Error **errp) > */ > static int zlib_recv_setup(MultiFDRecvParams *p, Error **errp) > { > - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); > struct zlib_data *z = g_malloc0(sizeof(struct zlib_data)); > z_stream *zs = &z->zs; > > @@ -194,10 +191,8 @@ static int zlib_recv_setup(MultiFDRecvParams *p, Error **errp) > error_setg(errp, "multifd %d: inflate init failed", p->id); > return -1; > } > - /* We will never have more than page_count pages */ > - z->zbuff_len = page_count * qemu_target_page_size(); > - /* We know compression "could" use more space */ > - z->zbuff_len *= 2; > + /* To be safe, we reserve twice the size of the packet */ > + z->zbuff_len = MULTIFD_PACKET_SIZE * 2; > z->zbuff = g_try_malloc(z->zbuff_len); > if (!z->zbuff) { > inflateEnd(zs); > diff --git a/migration/multifd-zstd.c b/migration/multifd-zstd.c > index 693bddf8c9..cc3b8869c0 100644 > --- a/migration/multifd-zstd.c > +++ b/migration/multifd-zstd.c > @@ -47,7 +47,6 @@ struct zstd_data { > */ > static int zstd_send_setup(MultiFDSendParams *p, Error **errp) > { > - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); > struct zstd_data *z = g_new0(struct zstd_data, 1); > int res; > > @@ -67,9 +66,8 @@ static int zstd_send_setup(MultiFDSendParams *p, Error **errp) > p->id, ZSTD_getErrorName(res)); > return -1; > } > - /* We will never have more than page_count pages */ > - z->zbuff_len = page_count * qemu_target_page_size(); > - z->zbuff_len *= 2; > + /* To be safe, we reserve twice the size of the packet */ > + z->zbuff_len = MULTIFD_PACKET_SIZE * 2; > z->zbuff = g_try_malloc(z->zbuff_len); > if (!z->zbuff) { > ZSTD_freeCStream(z->zcs); > @@ -191,7 +189,6 @@ static int zstd_send_write(MultiFDSendParams *p, uint32_t used, Error **errp) > */ > static int zstd_recv_setup(MultiFDRecvParams *p, Error **errp) > { > - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); > struct zstd_data *z = g_new0(struct zstd_data, 1); > int ret; > > @@ -212,10 +209,8 @@ static int zstd_recv_setup(MultiFDRecvParams *p, Error **errp) > return -1; > } > > - /* We will never have more than page_count pages */ > - z->zbuff_len = page_count * qemu_target_page_size(); > - /* We know compression "could" use more space */ > - z->zbuff_len *= 2; > + /* To be safe, we reserve twice the size of the packet */ > + z->zbuff_len = MULTIFD_PACKET_SIZE * 2; > z->zbuff = g_try_malloc(z->zbuff_len); > if (!z->zbuff) { > ZSTD_freeDStream(z->zds); > -- > 2.33.1 >
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> We are divining by page_size to multiply again in the only use. > ^--- typo >> Once there, impreve the comments. > ^--- typo >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> > > OK, with the typo's fixed: Thanks. > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > but, could you also explain the x 2 (that's no worse than the current > code); is this defined somewhere in zlib? I thought there was a routine > that told you the worst case? Nowhere. There are pathological cases where it can be worse. Not clear at all how much (ok, for zlib it appears that it is on the order of dozen of bytes, because it marks it as uncompressed on the worst possible case), For zstd, there is not a clear/fast answer when you google. As this buffer is held for the whole migration, it is one for thread, this looked safe to me. Notice that we are compressing 128 pages at a time, so for it not to compress anything looks very pathological. But as one says, better safe than sorry. If anyone that knows more about zlib/zstd give me different values, I will change that in an additional patch. Later, Juan.
* Juan Quintela (quintela@redhat.com) wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > > * Juan Quintela (quintela@redhat.com) wrote: > >> We are divining by page_size to multiply again in the only use. > > ^--- typo > >> Once there, impreve the comments. > > ^--- typo > >> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > > > > OK, with the typo's fixed: > > Thanks. > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > but, could you also explain the x 2 (that's no worse than the current > > code); is this defined somewhere in zlib? I thought there was a routine > > that told you the worst case? > > Nowhere. > > There are pathological cases where it can be worse. Not clear at all > how much (ok, for zlib it appears that it is on the order of dozen of > bytes, because it marks it as uncompressed on the worst possible case), > For zstd, there is not a clear/fast answer when you google. For zlib: ZEXTERN uLong ZEXPORT compressBound OF((uLong sourceLen)); /* compressBound() returns an upper bound on the compressed size after compress() or compress2() on sourceLen bytes. It would be used before a compress() or compress2() call to allocate the destination buffer. */ > As this buffer is held for the whole migration, it is one for thread, > this looked safe to me. Notice that we are compressing 128 pages at a > time, so for it not to compress anything looks very pathological. > > But as one says, better safe than sorry. Yeh. Dave > If anyone that knows more about zlib/zstd give me different values, I > will change that in an additional patch. > > Later, Juan. >
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Juan Quintela (quintela@redhat.com) wrote: >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: >> > * Juan Quintela (quintela@redhat.com) wrote: >> >> We are divining by page_size to multiply again in the only use. >> > ^--- typo >> >> Once there, impreve the comments. >> > ^--- typo >> >> >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> > >> > OK, with the typo's fixed: >> >> Thanks. >> >> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> > >> > but, could you also explain the x 2 (that's no worse than the current >> > code); is this defined somewhere in zlib? I thought there was a routine >> > that told you the worst case? >> >> Nowhere. >> >> There are pathological cases where it can be worse. Not clear at all >> how much (ok, for zlib it appears that it is on the order of dozen of >> bytes, because it marks it as uncompressed on the worst possible case), >> For zstd, there is not a clear/fast answer when you google. > > For zlib: > > ZEXTERN uLong ZEXPORT compressBound OF((uLong sourceLen)); > /* > compressBound() returns an upper bound on the compressed size after > compress() or compress2() on sourceLen bytes. It would be used before a > compress() or compress2() call to allocate the destination buffer. > */ Aha, exaactly what I needed. thanks. zstd one is called: ZSTD_compressBound() Added to the series. Thanks, Juan.
diff --git a/migration/multifd-zlib.c b/migration/multifd-zlib.c index ab4ba75d75..3fc7813b44 100644 --- a/migration/multifd-zlib.c +++ b/migration/multifd-zlib.c @@ -42,7 +42,6 @@ struct zlib_data { */ static int zlib_send_setup(MultiFDSendParams *p, Error **errp) { - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); struct zlib_data *z = g_malloc0(sizeof(struct zlib_data)); z_stream *zs = &z->zs; @@ -54,9 +53,8 @@ static int zlib_send_setup(MultiFDSendParams *p, Error **errp) error_setg(errp, "multifd %d: deflate init failed", p->id); return -1; } - /* We will never have more than page_count pages */ - z->zbuff_len = page_count * qemu_target_page_size(); - z->zbuff_len *= 2; + /* To be safe, we reserve twice the size of the packet */ + z->zbuff_len = MULTIFD_PACKET_SIZE * 2; z->zbuff = g_try_malloc(z->zbuff_len); if (!z->zbuff) { deflateEnd(&z->zs); @@ -180,7 +178,6 @@ static int zlib_send_write(MultiFDSendParams *p, uint32_t used, Error **errp) */ static int zlib_recv_setup(MultiFDRecvParams *p, Error **errp) { - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); struct zlib_data *z = g_malloc0(sizeof(struct zlib_data)); z_stream *zs = &z->zs; @@ -194,10 +191,8 @@ static int zlib_recv_setup(MultiFDRecvParams *p, Error **errp) error_setg(errp, "multifd %d: inflate init failed", p->id); return -1; } - /* We will never have more than page_count pages */ - z->zbuff_len = page_count * qemu_target_page_size(); - /* We know compression "could" use more space */ - z->zbuff_len *= 2; + /* To be safe, we reserve twice the size of the packet */ + z->zbuff_len = MULTIFD_PACKET_SIZE * 2; z->zbuff = g_try_malloc(z->zbuff_len); if (!z->zbuff) { inflateEnd(zs); diff --git a/migration/multifd-zstd.c b/migration/multifd-zstd.c index 693bddf8c9..cc3b8869c0 100644 --- a/migration/multifd-zstd.c +++ b/migration/multifd-zstd.c @@ -47,7 +47,6 @@ struct zstd_data { */ static int zstd_send_setup(MultiFDSendParams *p, Error **errp) { - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); struct zstd_data *z = g_new0(struct zstd_data, 1); int res; @@ -67,9 +66,8 @@ static int zstd_send_setup(MultiFDSendParams *p, Error **errp) p->id, ZSTD_getErrorName(res)); return -1; } - /* We will never have more than page_count pages */ - z->zbuff_len = page_count * qemu_target_page_size(); - z->zbuff_len *= 2; + /* To be safe, we reserve twice the size of the packet */ + z->zbuff_len = MULTIFD_PACKET_SIZE * 2; z->zbuff = g_try_malloc(z->zbuff_len); if (!z->zbuff) { ZSTD_freeCStream(z->zcs); @@ -191,7 +189,6 @@ static int zstd_send_write(MultiFDSendParams *p, uint32_t used, Error **errp) */ static int zstd_recv_setup(MultiFDRecvParams *p, Error **errp) { - uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size(); struct zstd_data *z = g_new0(struct zstd_data, 1); int ret; @@ -212,10 +209,8 @@ static int zstd_recv_setup(MultiFDRecvParams *p, Error **errp) return -1; } - /* We will never have more than page_count pages */ - z->zbuff_len = page_count * qemu_target_page_size(); - /* We know compression "could" use more space */ - z->zbuff_len *= 2; + /* To be safe, we reserve twice the size of the packet */ + z->zbuff_len = MULTIFD_PACKET_SIZE * 2; z->zbuff = g_try_malloc(z->zbuff_len); if (!z->zbuff) { ZSTD_freeDStream(z->zds);
We are divining by page_size to multiply again in the only use. Once there, impreve the comments. Signed-off-by: Juan Quintela <quintela@redhat.com> --- migration/multifd-zlib.c | 13 ++++--------- migration/multifd-zstd.c | 13 ++++--------- 2 files changed, 8 insertions(+), 18 deletions(-)