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

[hao . xiang]

> 
> On Sun, Mar 3, 2024 at 11:46 PM Peter Xu <peterx@redhat.com> wrote:
> 
> > 
> > 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?

So if multifd is enabled, the new parameter takes effect. If multifd is not 
enabled, zero page checking will always be done in the main thread, which is 
exactly the behavior it is now. I thought legacy migration is a deprecated 
feature so I am trying to not add new stuff to it.

> > 
> >  -
> > 
> >  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.

Will drop that.

> > 
> >  + * 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
> >
>