migration: Don't return for postcopy_send_discard_bm_ram()

[PATCH] migration: Don't return for postcopy_send_discard_bm_ram()

Posted by Philippe Mathieu-Daudé 4 years, 1 month ago

postcopy_send_discard_bm_ram() always return zero. Since it can't
fail, simplify and do not return anything.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Based-on: <20211224065000.97572-1-peterx@redhat.com>
---
 migration/ram.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 5234d1ece11..e241ce98461 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2433,14 +2433,12 @@ void ram_postcopy_migrated_memory_release(MigrationState *ms)
 /**
  * postcopy_send_discard_bm_ram: discard a RAMBlock
  *
- * Returns zero on success
- *
  * Callback from postcopy_each_ram_send_discard for each RAMBlock
  *
  * @ms: current migration state
  * @block: RAMBlock to discard
  */
-static int postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
+static void postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
 {
     unsigned long end = block->used_length >> TARGET_PAGE_BITS;
     unsigned long current;
@@ -2464,8 +2462,6 @@ static int postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
         postcopy_discard_send_range(ms, one, discard_length);
         current = one + discard_length;
     }
-
-    return 0;
 }
 
 static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block);
-- 
2.33.1

Re: [PATCH] migration: Don't return for postcopy_send_discard_bm_ram()

Posted by David Edmondson 4 years, 1 month ago

On Thursday, 2021-12-30 at 17:05:25 +01, Philippe Mathieu-Daudé wrote:

> postcopy_send_discard_bm_ram() always return zero. Since it can't
> fail, simplify and do not return anything.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
> Based-on: <20211224065000.97572-1-peterx@redhat.com>
> ---
>  migration/ram.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 5234d1ece11..e241ce98461 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2433,14 +2433,12 @@ void ram_postcopy_migrated_memory_release(MigrationState *ms)
>  /**
>   * postcopy_send_discard_bm_ram: discard a RAMBlock
>   *
> - * Returns zero on success
> - *
>   * Callback from postcopy_each_ram_send_discard for each RAMBlock
>   *
>   * @ms: current migration state
>   * @block: RAMBlock to discard
>   */
> -static int postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
> +static void postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
>  {
>      unsigned long end = block->used_length >> TARGET_PAGE_BITS;
>      unsigned long current;
> @@ -2464,8 +2462,6 @@ static int postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
>          postcopy_discard_send_range(ms, one, discard_length);
>          current = one + discard_length;
>      }
> -
> -    return 0;
>  }
>
>  static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block);

dme.
-- 
Tell me sweet little lies.

Re: [PATCH] migration: Don't return for postcopy_send_discard_bm_ram()

Posted by Juan Quintela 4 years ago

Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> postcopy_send_discard_bm_ram() always return zero. Since it can't
> fail, simplify and do not return anything.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Based-on: <20211224065000.97572-1-peterx@redhat.com>

And here, I learn to read Based-on:

Reviewed-by: Juan Quintela <quintela@redhat.com>

queued.

Re: [PATCH] migration: Don't return for postcopy_send_discard_bm_ram()

Posted by Juan Quintela 4 years, 1 month ago

Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> postcopy_send_discard_bm_ram() always return zero. Since it can't
> fail, simplify and do not return anything.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Based-on: <20211224065000.97572-1-peterx@redhat.com>
> ---
>  migration/ram.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 5234d1ece11..e241ce98461 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2433,14 +2433,12 @@ void ram_postcopy_migrated_memory_release(MigrationState *ms)
>  /**
>   * postcopy_send_discard_bm_ram: discard a RAMBlock
>   *
> - * Returns zero on success
> - *
>   * Callback from postcopy_each_ram_send_discard for each RAMBlock
>   *
>   * @ms: current migration state
>   * @block: RAMBlock to discard
>   */
> -static int postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
> +static void postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
>  {
>      unsigned long end = block->used_length >> TARGET_PAGE_BITS;
>      unsigned long current;
> @@ -2464,8 +2462,6 @@ static int postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
>          postcopy_discard_send_range(ms, one, discard_length);
>          current = one + discard_length;
>      }
> -
> -    return 0;
>  }
>  
>  static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block);

Nack.

You need to change the only caller (postcopy_each_send_discard) also.

        ret = postcopy_send_discard_bm_ram(ms, block);
        postcopy_discard_send_finish(ms);
        if (ret) {
            return ret;
        }

Not sure if doing the same operation with
postcopy_each_send_discard/ram_postcopy_send_discard_bitmap() and
postcopy_chunk_hugepages makes sense.

Later, Juan.

Re: [PATCH] migration: Don't return for postcopy_send_discard_bm_ram()

Posted by Peter Xu 4 years, 1 month ago

On Tue, Jan 04, 2022 at 10:15:16AM +0100, Juan Quintela wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > postcopy_send_discard_bm_ram() always return zero. Since it can't
> > fail, simplify and do not return anything.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> > Based-on: <20211224065000.97572-1-peterx@redhat.com>
> > ---
> >  migration/ram.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 5234d1ece11..e241ce98461 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -2433,14 +2433,12 @@ void ram_postcopy_migrated_memory_release(MigrationState *ms)
> >  /**
> >   * postcopy_send_discard_bm_ram: discard a RAMBlock
> >   *
> > - * Returns zero on success
> > - *
> >   * Callback from postcopy_each_ram_send_discard for each RAMBlock
> >   *
> >   * @ms: current migration state
> >   * @block: RAMBlock to discard
> >   */
> > -static int postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
> > +static void postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
> >  {
> >      unsigned long end = block->used_length >> TARGET_PAGE_BITS;
> >      unsigned long current;
> > @@ -2464,8 +2462,6 @@ static int postcopy_send_discard_bm_ram(MigrationState *ms, RAMBlock *block)
> >          postcopy_discard_send_range(ms, one, discard_length);
> >          current = one + discard_length;
> >      }
> > -
> > -    return 0;
> >  }
> >  
> >  static void postcopy_chunk_hostpages_pass(MigrationState *ms, RAMBlock *block);
> 
> Nack.
> 
> You need to change the only caller (postcopy_each_send_discard) also.
> 
>         ret = postcopy_send_discard_bm_ram(ms, block);
>         postcopy_discard_send_finish(ms);
>         if (ret) {
>             return ret;
>         }
> 
> Not sure if doing the same operation with
> postcopy_each_send_discard/ram_postcopy_send_discard_bitmap() and
> postcopy_chunk_hugepages makes sense.

Juan,

Phil's patch has a based-on dependency with the other patch:

https://lore.kernel.org/qemu-devel/20211224065000.97572-6-peterx@redhat.com/

Thanks,

-- 
Peter Xu