[Qemu-devel] [PATCH 0/2] fix the flaws of qemu_put_compression_data

[Qemu-devel] [PATCH 0/2] fix the flaws of qemu_put_compression_data

[Liang Li]

This patch fixed the flaws in qemu_put_compression_data function.
and cleanup the code based on the change.

Liang Li (2):
  qemu-file: fix flaws of qemu_put_compression_data
  migration: code clean up.

 migration/qemu-file.c | 10 +++++++++-
 migration/ram.c       | 20 ++++++++------------
 2 files changed, 17 insertions(+), 13 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] qemu-file: fix flaws of qemu_put_compression_data

[Liang Li]

There are some flaws in qemu_put_compression_data, this patch tries
to fix it. Now it can be used by other code.

Signed-off-by: Liang Li <liang.z.li@intel.com>
---
 migration/qemu-file.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 0bbd257..ef9cd4a 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -616,7 +616,9 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
     ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
 
     if (blen < compressBound(size)) {
-        return 0;
+        if (f->ops->writev_buffer || f->ops->put_buffer) {
+            qemu_fflush(f);
+        }
     }
     if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
                   (Bytef *)p, size, level) != Z_OK) {
@@ -624,7 +626,13 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
         return 0;
     }
     qemu_put_be32(f, blen);
+    if (f->ops->writev_buffer) {
+        add_to_iovec(f, f->buf + f->buf_index, blen);
+    }
     f->buf_index += blen;
+    if (f->buf_index == IO_BUF_SIZE) {
+        qemu_fflush(f);
+    }
     return blen + sizeof(int32_t);
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] migration: code clean up.

[Liang Li]

Use qemu_put_compression_data to do the compression directly
instead of using do_compress_ram_page, avoid some data copy.
very small improvement, but the code looks better.

Signed-off-by: Liang Li <liang.z.li@intel.com>
---
 migration/ram.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 1eb155a..44b3edc 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -911,22 +911,18 @@ static int ram_save_compressed_page(QEMUFile *f, RAMBlock *block,
                                     uint64_t *bytes_transferred)
 {
     int pages = -1;
-    uint64_t bytes_xmit;
+    uint64_t bytes_xmit = 0;
     uint8_t *p;
     int ret;
 
     p = block->host + offset;
 
-    bytes_xmit = 0;
     ret = ram_control_save_page(f, block->offset,
                                 offset, TARGET_PAGE_SIZE, &bytes_xmit);
     if (bytes_xmit) {
         *bytes_transferred += bytes_xmit;
         pages = 1;
     }
-    if (block == last_sent_block) {
-        offset |= RAM_SAVE_FLAG_CONTINUE;
-    }
     if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
         if (ret != RAM_SAVE_CONTROL_DELAYED) {
             if (bytes_xmit > 0) {
@@ -946,17 +942,17 @@ static int ram_save_compressed_page(QEMUFile *f, RAMBlock *block,
             flush_compressed_data(f);
             pages = save_zero_page(f, block, offset, p, bytes_transferred);
             if (pages == -1) {
-                set_compress_params(&comp_param[0], block, offset);
-                /* Use the qemu thread to compress the data to make sure the
-                 * first page is sent out before other pages
-                 */
-                bytes_xmit = do_compress_ram_page(&comp_param[0]);
-                acct_info.norm_pages++;
-                qemu_put_qemu_file(f, comp_param[0].file);
+                /* Make sure the first page is sent out before other pages */
+                bytes_xmit = save_page_header(f, block, offset |
+                                              RAM_SAVE_FLAG_COMPRESS_PAGE);
+                bytes_xmit += qemu_put_compression_data(f, p, TARGET_PAGE_SIZE,
+                                                migrate_compress_level());
                 *bytes_transferred += bytes_xmit;
+                acct_info.norm_pages++;
                 pages = 1;
             }
         } else {
+            offset |= RAM_SAVE_FLAG_CONTINUE;
             pages = save_zero_page(f, block, offset, p, bytes_transferred);
             if (pages == -1) {
                 pages = compress_page_with_multi_thread(f, block, offset,
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] migration: code clean up.

[Liang Li]

Use qemu_put_compression_data to do the compression directly
instead of using do_compress_ram_page, avoid some data copy.
very small improvement, but the code looks better.

Signed-off-by: Liang Li <liang.z.li@intel.com>
---
 migration/ram.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 1eb155a..44b3edc 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -911,22 +911,18 @@ static int ram_save_compressed_page(QEMUFile *f, RAMBlock *block,
                                     uint64_t *bytes_transferred)
 {
     int pages = -1;
-    uint64_t bytes_xmit;
+    uint64_t bytes_xmit = 0;
     uint8_t *p;
     int ret;
 
     p = block->host + offset;
 
-    bytes_xmit = 0;
     ret = ram_control_save_page(f, block->offset,
                                 offset, TARGET_PAGE_SIZE, &bytes_xmit);
     if (bytes_xmit) {
         *bytes_transferred += bytes_xmit;
         pages = 1;
     }
-    if (block == last_sent_block) {
-        offset |= RAM_SAVE_FLAG_CONTINUE;
-    }
     if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
         if (ret != RAM_SAVE_CONTROL_DELAYED) {
             if (bytes_xmit > 0) {
@@ -946,17 +942,17 @@ static int ram_save_compressed_page(QEMUFile *f, RAMBlock *block,
             flush_compressed_data(f);
             pages = save_zero_page(f, block, offset, p, bytes_transferred);
             if (pages == -1) {
-                set_compress_params(&comp_param[0], block, offset);
-                /* Use the qemu thread to compress the data to make sure the
-                 * first page is sent out before other pages
-                 */
-                bytes_xmit = do_compress_ram_page(&comp_param[0]);
-                acct_info.norm_pages++;
-                qemu_put_qemu_file(f, comp_param[0].file);
+                /* Make sure the first page is sent out before other pages */
+                bytes_xmit = save_page_header(f, block, offset |
+                                              RAM_SAVE_FLAG_COMPRESS_PAGE);
+                bytes_xmit += qemu_put_compression_data(f, p, TARGET_PAGE_SIZE,
+                                                migrate_compress_level());
                 *bytes_transferred += bytes_xmit;
+                acct_info.norm_pages++;
                 pages = 1;
             }
         } else {
+            offset |= RAM_SAVE_FLAG_CONTINUE;
             pages = save_zero_page(f, block, offset, p, bytes_transferred);
             if (pages == -1) {
                 pages = compress_page_with_multi_thread(f, block, offset,
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/2] fix the flaws of qemu_put_compression_data

[Juan Quintela]

Liang Li <liang.z.li@intel.com> wrote:
> This patch fixed the flaws in qemu_put_compression_data function.
> and cleanup the code based on the change.

Hi

We are in hard freeze.  My understanding is that this are
"optimizations" that can wait for 2.6:
- my understanding from commit from message one and from quick look at
  the code is that this change is not needed for current users, is that correct?
- we avoid a copy at the beginning of each block (micro-optimization)

If my understanding is correct, I am delaying this two patches to 2.6.

What do you think?

Later, Juan.

BTW, amit address was wrongly typed, I just fixed it.


>
> Liang Li (2):
>   qemu-file: fix flaws of qemu_put_compression_data
>   migration: code clean up.
>
>  migration/qemu-file.c | 10 +++++++++-
>  migration/ram.c       | 20 ++++++++------------
>  2 files changed, 17 insertions(+), 13 deletions(-)

Re: [Qemu-devel] [PATCH 1/2] qemu-file: fix flaws of qemu_put_compression_data

[Juan Quintela]

Liang Li <liang.z.li@intel.com> wrote:
> There are some flaws in qemu_put_compression_data, this patch tries
> to fix it. Now it can be used by other code.
>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> ---
>  migration/qemu-file.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 0bbd257..ef9cd4a 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -616,7 +616,9 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
>      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
>  
>      if (blen < compressBound(size)) {
> -        return 0;
> +        if (f->ops->writev_buffer || f->ops->put_buffer) {
> +            qemu_fflush(f);
> +        }
>      }

With your change, when we arrive here:

- blen could still be smaller that compressBound(size), you need to
  recheck
- blen could have changed, but you don't take that in account for the
  following caller.

So, I think code has a bug?

Later, Juan.

>      if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
>                    (Bytef *)p, size, level) != Z_OK) {
> @@ -624,7 +626,13 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
>          return 0;
>      }
>      qemu_put_be32(f, blen);
> +    if (f->ops->writev_buffer) {
> +        add_to_iovec(f, f->buf + f->buf_index, blen);
> +    }
>      f->buf_index += blen;
> +    if (f->buf_index == IO_BUF_SIZE) {
> +        qemu_fflush(f);
> +    }
>      return blen + sizeof(int32_t);
>  }

Re: [Qemu-devel] [PATCH 0/2] fix the flaws of qemu_put_compression_data

[Li, Liang Z]

> Hi
> 
> We are in hard freeze.  My understanding is that this are "optimizations" that
> can wait for 2.6:
> - my understanding from commit from message one and from quick look at
>   the code is that this change is not needed for current users, is that correct?
> - we avoid a copy at the beginning of each block (micro-optimization)
> 
> If my understanding is correct, I am delaying this two patches to 2.6.
> 
> What do you think?
> 

Yes, you are wright. I think it's ok to delay to 2.6.

Thanks.

Liang.

Re: [Qemu-devel] [PATCH 1/2] qemu-file: fix flaws of qemu_put_compression_data

[Li, Liang Z]

> > There are some flaws in qemu_put_compression_data, this patch tries to
> > fix it. Now it can be used by other code.
> >
> > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > ---
> >  migration/qemu-file.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c index
> > 0bbd257..ef9cd4a 100644
> > --- a/migration/qemu-file.c
> > +++ b/migration/qemu-file.c
> > @@ -616,7 +616,9 @@ ssize_t qemu_put_compression_data(QEMUFile *f,
> const uint8_t *p, size_t size,
> >      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
> >
> >      if (blen < compressBound(size)) {
> > -        return 0;
> > +        if (f->ops->writev_buffer || f->ops->put_buffer) {
> > +            qemu_fflush(f);
> > +        }
> >      }
> 
> With your change, when we arrive here:
> 
> - blen could still be smaller that compressBound(size), you need to
>   recheck
> - blen could have changed, but you don't take that in account for the
>   following caller.
> 
> So, I think code has a bug?

Yes, there is a bug, I should consider the case QEMUFile with empty ops.
The right code should be like:

if (blen < compressBound(size)) {
        if (f->ops->writev_buffer || f->ops->put_buffer) {
            qemu_fflush(f);
        } else {
            return 0;
        }
}
....

It is enough?

Liang



> 
> Later, Juan.

Re: [Qemu-devel] [PATCH 1/2] qemu-file: fix flaws of qemu_put_compression_data

[Juan Quintela]

"Li, Liang Z" <liang.z.li@intel.com> wrote:
>> > There are some flaws in qemu_put_compression_data, this patch tries to
>> > fix it. Now it can be used by other code.
>> >
>> > Signed-off-by: Liang Li <liang.z.li@intel.com>
>> > ---
>> >  migration/qemu-file.c | 10 +++++++++-
>> >  1 file changed, 9 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c index
>> > 0bbd257..ef9cd4a 100644
>> > --- a/migration/qemu-file.c
>> > +++ b/migration/qemu-file.c
>> > @@ -616,7 +616,9 @@ ssize_t qemu_put_compression_data(QEMUFile *f,
>> const uint8_t *p, size_t size,
>> >      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
>> >
>> >      if (blen < compressBound(size)) {
>> > -        return 0;
>> > +        if (f->ops->writev_buffer || f->ops->put_buffer) {
>> > +            qemu_fflush(f);
>> > +        }
>> >      }
>> 
>> With your change, when we arrive here:
>> 
>> - blen could still be smaller that compressBound(size), you need to
>>   recheck
>> - blen could have changed, but you don't take that in account for the
>>   following caller.
>> 
>> So, I think code has a bug?
>
> Yes, there is a bug, I should consider the case QEMUFile with empty ops.
> The right code should be like:
>
> if (blen < compressBound(size)) {
>         if (f->ops->writev_buffer || f->ops->put_buffer) {
>             qemu_fflush(f);
>         } else {
>             return 0;
>         }
> }
> ....
>
> It is enough?

No.  We need something like:

    if (blen < compressBound(size)) {
         if (!f->ops->writev_buffer && !f->ops->put_buffer) {
             return 0;
         }
         qemu_fflush(f);
         blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
         if (blen < compressBound(size)) {
             return 0;
         }
}


No?


>
> Liang
>
>
>
>> 
>> Later, Juan.

Re: [Qemu-devel] [PATCH 1/2] qemu-file: fix flaws of qemu_put_compression_data

[Li, Liang Z]

> >> - blen could still be smaller that compressBound(size), you need to
> >>   recheck
> >> - blen could have changed, but you don't take that in account for the
> >>   following caller.
> >>
> >> So, I think code has a bug?
> >
> > Yes, there is a bug, I should consider the case QEMUFile with empty ops.
> > The right code should be like:
> >
> > if (blen < compressBound(size)) {
> >         if (f->ops->writev_buffer || f->ops->put_buffer) {
> >             qemu_fflush(f);
> >         } else {
> >             return 0;
> >         }
> > }
> > ....
> >
> > It is enough?
> 
> No.  We need something like:
> 
>     if (blen < compressBound(size)) {
>          if (!f->ops->writev_buffer && !f->ops->put_buffer) {
>              return 0;
>          }
>          qemu_fflush(f);
>          blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
>          if (blen < compressBound(size)) {
>              return 0;
>          }
> }
> 
> 
> No?
> 

I got it.  You mean we should not only consider the 'f' is not writable, but also  the case where 'size'  is a big value.  
You are right.  I will change it. Thanks.

Liang

Re: [Qemu-devel] [PATCH 1/2] qemu-file: fix flaws of qemu_put_compression_data

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

On Fri, Dec 04, 2015 at 11:52:07AM +0800, Liang Li wrote:
> There are some flaws in qemu_put_compression_data, this patch tries
> to fix it. Now it can be used by other code.

This commit description is vague and doesn't explain what this patch
does or why it is necessary.

Please try to cover "what" and "why" instead of just saying that the
patch fixes flaws.  If there are multiple points then maybe it should be
split into multiple patches.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 1/2] qemu-file: fix flaws of qemu_put_compression_data

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 417 bytes --]

On Fri, Dec 04, 2015 at 11:52:07AM +0800, Liang Li wrote:
> There are some flaws in qemu_put_compression_data, this patch tries
> to fix it. Now it can be used by other code.
> 
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> ---
>  migration/qemu-file.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

Ah, I see Amit made the same comment on the next revision of the patch.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 1/2] qemu-file: fix flaws of qemu_put_compression_data

[Li, Liang Z]

> On Fri, Dec 04, 2015 at 11:52:07AM +0800, Liang Li wrote:
> > There are some flaws in qemu_put_compression_data, this patch tries to
> > fix it. Now it can be used by other code.
> >
> > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > ---
> >  migration/qemu-file.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> Ah, I see Amit made the same comment on the next revision of the patch.
> 
> Stefan

Yes, it's really not clear, I will change it.