Re: [PATCH v4 3/7] migration/multifd: Implement ram_save_target_page_multifd to handle multifd version of MigrationOps::ram_save_target_page.

[Peter Xu <peterx@redhat.com>]

On Fri, Mar 01, 2024 at 02:28:25AM +0000, Hao Xiang wrote:
> 1. Add a dedicated handler for MigrationOps::ram_save_target_page in
> multifd live migration.
> 2. Refactor ram_save_target_page_legacy so that the legacy and multifd
> handlers don't have internal functions calling into each other.
> 
> Signed-off-by: Hao Xiang <hao.xiang@bytedance.com>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Message-Id: <20240226195654.934709-4-hao.xiang@bytedance.com>
> ---
>  migration/ram.c | 43 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index e1fa229acf..f9d6ea65cc 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1122,10 +1122,6 @@ static int save_zero_page(RAMState *rs, PageSearchStatus *pss,
>      QEMUFile *file = pss->pss_channel;
>      int len = 0;
>  
> -    if (migrate_zero_page_detection() == ZERO_PAGE_DETECTION_NONE) {
> -        return 0;
> -    }

We need to keep this to disable zero-page-detect on !multifd?

> -
>      if (!buffer_is_zero(p, TARGET_PAGE_SIZE)) {
>          return 0;
>      }
> @@ -2045,7 +2041,6 @@ static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
>   */
>  static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
>  {
> -    RAMBlock *block = pss->block;
>      ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
>      int res;
>  
> @@ -2061,17 +2056,34 @@ static int ram_save_target_page_legacy(RAMState *rs, PageSearchStatus *pss)
>          return 1;
>      }
>  
> +    return ram_save_page(rs, pss);
> +}
> +
> +/**
> + * ram_save_target_page_multifd: send one target page to multifd workers
> + *
> + * Returns 1 if the page was queued, -1 otherwise.
> + *
> + * @rs: current RAM state
> + * @pss: data about the page we want to send
> + */
> +static int ram_save_target_page_multifd(RAMState *rs, PageSearchStatus *pss)
> +{
> +    RAMBlock *block = pss->block;
> +    ram_addr_t offset = ((ram_addr_t)pss->page) << TARGET_PAGE_BITS;
> +
>      /*
> -     * Do not use multifd in postcopy as one whole host page should be
> -     * placed.  Meanwhile postcopy requires atomic update of pages, so even
> -     * if host page size == guest page size the dest guest during run may
> -     * still see partially copied pages which is data corruption.
> +     * Backward compatibility support. While using multifd live

We can also avoid mentioning "compatibility support" here - it's a
parameter, user can legally set it to anything.

> +     * migration, we still need to handle zero page checking on the
> +     * migration main thread.
>       */
> -    if (migrate_multifd() && !migration_in_postcopy()) {
> -        return ram_save_multifd_page(block, offset);
> +    if (migrate_zero_page_detection() == ZERO_PAGE_DETECTION_LEGACY) {
> +        if (save_zero_page(rs, pss, offset)) {
> +            return 1;
> +        }
>      }
>  
> -    return ram_save_page(rs, pss);
> +    return ram_save_multifd_page(block, offset);
>  }
>  
>  /* Should be called before sending a host page */
> @@ -2983,7 +2995,12 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      }
>  
>      migration_ops = g_malloc0(sizeof(MigrationOps));
> -    migration_ops->ram_save_target_page = ram_save_target_page_legacy;
> +
> +    if (migrate_multifd()) {
> +        migration_ops->ram_save_target_page = ram_save_target_page_multifd;
> +    } else {
> +        migration_ops->ram_save_target_page = ram_save_target_page_legacy;
> +    }
>  
>      bql_unlock();
>      ret = multifd_send_sync_main();
> -- 
> 2.30.2
> 

-- 
Peter Xu