[PATCH v2 5/6] migration: simplify do_compress_ram_page

Juan Quintela posted 6 patches 4 years, 1 month ago
Maintainers: Juan Quintela <quintela@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>
There is a newer version of this series
[PATCH v2 5/6] migration: simplify do_compress_ram_page
Posted by Juan Quintela 4 years, 1 month ago
The goto is not needed at all.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 4ee0369d6f..eddc85ffb0 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1341,12 +1341,11 @@ static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
 {
     RAMState *rs = ram_state;
     uint8_t *p = block->host + (offset & TARGET_PAGE_MASK);
-    bool zero_page = false;
     int ret;
 
     if (save_zero_page_to_file(rs, f, block, offset)) {
-        zero_page = true;
-        goto exit;
+        ram_release_page(block->idstr, offset & TARGET_PAGE_MASK);
+        return true;
     }
 
     save_page_header(rs, f, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE);
@@ -1361,12 +1360,8 @@ static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
     if (ret < 0) {
         qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
         error_report("compressed data failed!");
-        return false;
     }
-
-exit:
-    ram_release_page(block->idstr, offset);
-    return zero_page;
+    return false;
 }
 
 static void
-- 
2.33.1


Re: [PATCH v2 5/6] migration: simplify do_compress_ram_page
Posted by Philippe Mathieu-Daudé 4 years, 1 month ago
On 12/21/21 13:52, Juan Quintela wrote:
> The goto is not needed at all.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/ram.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 4ee0369d6f..eddc85ffb0 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1341,12 +1341,11 @@ static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
>  {
>      RAMState *rs = ram_state;
>      uint8_t *p = block->host + (offset & TARGET_PAGE_MASK);
> -    bool zero_page = false;
>      int ret;
>  
>      if (save_zero_page_to_file(rs, f, block, offset)) {
> -        zero_page = true;
> -        goto exit;
> +        ram_release_page(block->idstr, offset & TARGET_PAGE_MASK);

We don't want TARGET_PAGE_MASK anymore here, right?

> +        return true;
>      }
>  
>      save_page_header(rs, f, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE);
> @@ -1361,12 +1360,8 @@ static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
>      if (ret < 0) {
>          qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
>          error_report("compressed data failed!");
> -        return false;
>      }
> -
> -exit:
> -    ram_release_page(block->idstr, offset);
> -    return zero_page;
> +    return false;
>  }
>  
>  static void


Re: [PATCH v2 5/6] migration: simplify do_compress_ram_page
Posted by Peter Xu 4 years, 1 month ago
On Tue, Dec 21, 2021 at 02:29:13PM +0100, Philippe Mathieu-Daudé wrote:
> On 12/21/21 13:52, Juan Quintela wrote:
> > The goto is not needed at all.
> > 
> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> > ---
> >  migration/ram.c | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> > 
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 4ee0369d6f..eddc85ffb0 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -1341,12 +1341,11 @@ static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
> >  {
> >      RAMState *rs = ram_state;
> >      uint8_t *p = block->host + (offset & TARGET_PAGE_MASK);
> > -    bool zero_page = false;
> >      int ret;
> >  
> >      if (save_zero_page_to_file(rs, f, block, offset)) {
> > -        zero_page = true;
> > -        goto exit;
> > +        ram_release_page(block->idstr, offset & TARGET_PAGE_MASK);
> 
> We don't want TARGET_PAGE_MASK anymore here, right?

I suggest we simply do:

  offset &= TARGET_PAGE_MASK;

At the entry, then yes here. Meanwhile squash previous patch into this one;
that one smells half-done anyway..

Thanks,

> 
> > +        return true;
> >      }
> >  
> >      save_page_header(rs, f, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE);
> > @@ -1361,12 +1360,8 @@ static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
> >      if (ret < 0) {
> >          qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
> >          error_report("compressed data failed!");
> > -        return false;
> >      }
> > -
> > -exit:
> > -    ram_release_page(block->idstr, offset);
> > -    return zero_page;
> > +    return false;
> >  }
> >  
> >  static void
> 

-- 
Peter Xu