[PATCH v3 01/23] multifd: Delete useless operation

Juan Quintela posted 23 patches 4 years, 2 months ago
Maintainers: Juan Quintela <quintela@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>
There is a newer version of this series
[PATCH v3 01/23] multifd: Delete useless operation
Posted by Juan Quintela 4 years, 2 months ago
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(-)

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


Re: [PATCH v3 01/23] multifd: Delete useless operation
Posted by Dr. David Alan Gilbert 4 years, 2 months ago
* 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 / Manchester, UK


Re: [PATCH v3 01/23] multifd: Delete useless operation
Posted by Juan Quintela 4 years, 2 months ago
"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.