Re: [Qemu-devel] [PATCH 2/2] migration: extract ram_load_precopy

[Dr. David Alan Gilbert]

* Wei Yang (address@hidden) wrote:
> After cleanup, it would be clear to audience there are two cases
> ram_load:
> 
>   * precopy
>   * postcopy
> 
> And it is not necessary to check postcopy_running on each iteration for
> precopy.
> 
> Signed-off-by: Wei Yang <address@hidden>
> ---
>  migration/ram.c | 73 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 46 insertions(+), 27 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 6bfdfae16e..5f6f07b255 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4200,40 +4200,26 @@ static void colo_flush_ram_cache(void)
>      trace_colo_flush_ram_cache_end();
>  }
>  
> -static int ram_load(QEMUFile *f, void *opaque, int version_id)
> +/**
> + * ram_load_precopy: load a page in precopy case

This comment is wrong - although I realise you copied it from the
postcopy case; they don't just load a single page; they load 'pages'

Other than that, I think it's OK, so:


Reviewed-by: Dr. David Alan Gilbert <address@hidden>

> + * Returns 0 for success or -errno in case of error
> + *
> + * Called in precopy mode by ram_load().
> + * rcu_read_lock is taken prior to this being called.
> + *
> + * @f: QEMUFile where to send the data
> + */
> +static int ram_load_precopy(QEMUFile *f)
>  {
> -    int flags = 0, ret = 0, invalid_flags = 0;
> -    static uint64_t seq_iter;
> -    int len = 0;
> -    /*
> -     * If system is running in postcopy mode, page inserts to host memory 
> must
> -     * be atomic
> -     */
> -    bool postcopy_running = postcopy_is_running();
> +    int flags = 0, ret = 0, invalid_flags = 0, len = 0;
>      /* ADVISE is earlier, it shows the source has the postcopy capability on 
> */
>      bool postcopy_advised = postcopy_is_advised();
> -
> -    seq_iter++;
> -
> -    if (version_id != 4) {
> -        return -EINVAL;
> -    }
> -
>      if (!migrate_use_compression()) {
>          invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
>      }
> -    /* This RCU critical section can be very long running.
> -     * When RCU reclaims in the code start to become numerous,
> -     * it will be necessary to reduce the granularity of this
> -     * critical section.
> -     */
> -    rcu_read_lock();
> -
> -    if (postcopy_running) {
> -        ret = ram_load_postcopy(f);
> -    }
>  
> -    while (!postcopy_running && !ret && !(flags & RAM_SAVE_FLAG_EOS)) {
> +    while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
>          ram_addr_t addr, total_ram_bytes;
>          void *host = NULL;
>          uint8_t ch;
> @@ -4390,6 +4376,39 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>          }
>      }
>  
> +    return ret;
> +}
> +
> +static int ram_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +    int ret = 0;
> +    static uint64_t seq_iter;
> +    /*
> +     * If system is running in postcopy mode, page inserts to host memory 
> must
> +     * be atomic
> +     */
> +    bool postcopy_running = postcopy_is_running();
> +
> +    seq_iter++;
> +
> +    if (version_id != 4) {
> +        return -EINVAL;
> +    }
> +
> +    /*
> +     * This RCU critical section can be very long running.
> +     * When RCU reclaims in the code start to become numerous,
> +     * it will be necessary to reduce the granularity of this
> +     * critical section.
> +     */
> +    rcu_read_lock();
> +
> +    if (postcopy_running) {
> +        ret = ram_load_postcopy(f);
> +    } else {
> +        ret = ram_load_precopy(f);
> +    }
> +
>      ret |= wait_for_decompress_done();
>      rcu_read_unlock();
>      trace_ram_load_complete(ret, seq_iter);
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK