Re: [PATCH v2 1/2] live migration: do not use deffered bitmap when inappropriate

[Jan Beulich <jbeulich@suse.com>]

On 06.09.2022 11:54, Andrei Semenov wrote:
> Use deffered bitmap only in PV guests context as it not used for HVM guests.
> This allow to reduce memory pressure on domain0 while migrating very large
> (memory wise) HVM guests.
> 
> Signed-off-by: Andrei Semenov <andrei.semenov@vates.fr>

Did you see https://lists.xen.org/archives/html/xen-devel/2022-04/msg02037.html?
It would seem to me that doing what you want would be less intrusive on top of
that work, by simply suppressing the allocation also for HVM then.

Jan

> ---
>  tools/libs/guest/xg_sr_common.h       | 26 ++++++++++++++++--
>  tools/libs/guest/xg_sr_save.c         | 23 +++++++---------
>  tools/libs/guest/xg_sr_save_x86_hvm.c | 21 +++++++++++++++
>  tools/libs/guest/xg_sr_save_x86_pv.c  | 39 +++++++++++++++++++++++++++
>  4 files changed, 93 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
> index 36d45ef56f..941e24d7b7 100644
> --- a/tools/libs/guest/xg_sr_common.h
> +++ b/tools/libs/guest/xg_sr_common.h
> @@ -96,6 +96,24 @@ struct xc_sr_save_ops
>       */
>      int (*check_vm_state)(struct xc_sr_context *ctx);
>  
> +    /**
> +     * For some reasons the page can't be sent for the moment. Postpone this
> +     * send to the later stage when domain is suspended.
> +     */
> +    int (*defer_page)(struct xc_sr_context *ctx, xen_pfn_t pfn);
> +
> +    /**
> +     *  Merge all deferred pages with the dirty pages bitmap (in order to be
> +     *  sent).
> +     */
> +    int (*merge_deferred)(const struct xc_sr_context *ctx,
> +                          unsigned long *bitmap, unsigned long *count);
> +
> +    /**
> +     *  Deferred pages was successfully sent. Reset all associated information.
> +     */
> +    int (*reset_deferred)(struct xc_sr_context *ctx);
> +
>      /**
>       * Clean up the local environment.  Will be called exactly once, either
>       * after a successful save, or upon encountering an error.
> @@ -243,8 +261,6 @@ struct xc_sr_context
>  
>              xen_pfn_t *batch_pfns;
>              unsigned int nr_batch_pfns;
> -            unsigned long *deferred_pages;
> -            unsigned long nr_deferred_pages;
>              xc_hypercall_buffer_t dirty_bitmap_hbuf;
>          } save;
>  
> @@ -349,6 +365,12 @@ struct xc_sr_context
>  
>                  union
>                  {
> +                    struct
> +                    {
> +                        unsigned long *deferred_pages;
> +                        unsigned long nr_deferred_pages;
> +                    } save;
> +
>                      struct
>                      {
>                          /* State machine for the order of received records. */
> diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
> index 9853d8d846..602b18488d 100644
> --- a/tools/libs/guest/xg_sr_save.c
> +++ b/tools/libs/guest/xg_sr_save.c
> @@ -132,8 +132,7 @@ static int write_batch(struct xc_sr_context *ctx)
>          /* Likely a ballooned page. */
>          if ( mfns[i] == INVALID_MFN )
>          {
> -            set_bit(ctx->save.batch_pfns[i], ctx->save.deferred_pages);
> -            ++ctx->save.nr_deferred_pages;
> +            ctx->save.ops.defer_page(ctx, ctx->save.batch_pfns[i]);
>          }
>      }
>  
> @@ -192,8 +191,7 @@ static int write_batch(struct xc_sr_context *ctx)
>              {
>                  if ( rc == -1 && errno == EAGAIN )
>                  {
> -                    set_bit(ctx->save.batch_pfns[i], ctx->save.deferred_pages);
> -                    ++ctx->save.nr_deferred_pages;
> +                    ctx->save.ops.defer_page(ctx, ctx->save.batch_pfns[i]);
>                      types[i] = XEN_DOMCTL_PFINFO_XTAB;
>                      --nr_pages;
>                  }
> @@ -641,6 +639,7 @@ static int suspend_and_send_dirty(struct xc_sr_context *ctx)
>      xc_interface *xch = ctx->xch;
>      xc_shadow_op_stats_t stats = { 0, ctx->save.p2m_size };
>      char *progress_str = NULL;
> +    unsigned long merged;
>      int rc;
>      DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap,
>                                      &ctx->save.dirty_bitmap_hbuf);
> @@ -669,7 +668,7 @@ static int suspend_and_send_dirty(struct xc_sr_context *ctx)
>      else
>          xc_set_progress_prefix(xch, "Checkpointed save");
>  
> -    bitmap_or(dirty_bitmap, ctx->save.deferred_pages, ctx->save.p2m_size);
> +    ctx->save.ops.merge_deferred(ctx, dirty_bitmap, &merged);
>  
>      if ( !ctx->save.live && ctx->stream_type == XC_STREAM_COLO )
>      {
> @@ -681,12 +680,11 @@ static int suspend_and_send_dirty(struct xc_sr_context *ctx)
>          }
>      }
>  
> -    rc = send_dirty_pages(ctx, stats.dirty_count + ctx->save.nr_deferred_pages);
> +    rc = send_dirty_pages(ctx, stats.dirty_count + merged);
>      if ( rc )
>          goto out;
>  
> -    bitmap_clear(ctx->save.deferred_pages, ctx->save.p2m_size);
> -    ctx->save.nr_deferred_pages = 0;
> +    ctx->save.ops.reset_deferred(ctx);
>  
>   out:
>      xc_set_progress_prefix(xch, NULL);
> @@ -805,18 +803,16 @@ static int setup(struct xc_sr_context *ctx)
>          xch, dirty_bitmap, NRPAGES(bitmap_size(ctx->save.p2m_size)));
>      ctx->save.batch_pfns = malloc(MAX_BATCH_SIZE *
>                                    sizeof(*ctx->save.batch_pfns));
> -    ctx->save.deferred_pages = bitmap_alloc(ctx->save.p2m_size);
>  
> -    if ( !ctx->save.batch_pfns || !dirty_bitmap || !ctx->save.deferred_pages )
> +    if ( !ctx->save.batch_pfns || !dirty_bitmap )
>      {
> -        ERROR("Unable to allocate memory for dirty bitmaps, batch pfns and"
> -              " deferred pages");
> +        ERROR("Unable to allocate memory for dirty bitmaps, batch pfns");
>          rc = -1;
>          errno = ENOMEM;
>          goto err;
>      }
>  
> -    rc = 0;
> +    rc = ctx->save.ops.reset_deferred(ctx);
>  
>   err:
>      return rc;
> @@ -837,7 +833,6 @@ static void cleanup(struct xc_sr_context *ctx)
>  
>      xc_hypercall_buffer_free_pages(xch, dirty_bitmap,
>                                     NRPAGES(bitmap_size(ctx->save.p2m_size)));
> -    free(ctx->save.deferred_pages);
>      free(ctx->save.batch_pfns);
>  }
>  
> diff --git a/tools/libs/guest/xg_sr_save_x86_hvm.c b/tools/libs/guest/xg_sr_save_x86_hvm.c
> index 1634a7bc43..3c762a0af0 100644
> --- a/tools/libs/guest/xg_sr_save_x86_hvm.c
> +++ b/tools/libs/guest/xg_sr_save_x86_hvm.c
> @@ -211,6 +211,24 @@ static int x86_hvm_end_of_checkpoint(struct xc_sr_context *ctx)
>      return 0;
>  }
>  
> +static int x86_hvm_defer_page(struct xc_sr_context *ctx, xen_pfn_t pfn)
> +{
> +    return 0;
> +}
> +
> +static int x86_hvm_merge_deferred(const struct xc_sr_context *ctx,
> +                                 unsigned long *bitmap, unsigned long *count)
> +{
> +    *count = 0;
> +
> +    return 0;
> +}
> +
> +static int x86_hvm_reset_deferred(struct xc_sr_context *ctx)
> +{
> +    return 0;
> +}
> +
>  static int x86_hvm_cleanup(struct xc_sr_context *ctx)
>  {
>      xc_interface *xch = ctx->xch;
> @@ -237,6 +255,9 @@ struct xc_sr_save_ops save_ops_x86_hvm =
>      .start_of_checkpoint = x86_hvm_start_of_checkpoint,
>      .end_of_checkpoint   = x86_hvm_end_of_checkpoint,
>      .check_vm_state      = x86_hvm_check_vm_state,
> +    .defer_page          = x86_hvm_defer_page,
> +    .merge_deferred      = x86_hvm_merge_deferred,
> +    .reset_deferred      = x86_hvm_reset_deferred,
>      .cleanup             = x86_hvm_cleanup,
>  };
>  
> diff --git a/tools/libs/guest/xg_sr_save_x86_pv.c b/tools/libs/guest/xg_sr_save_x86_pv.c
> index 4964f1f7b8..5fdc7e9590 100644
> --- a/tools/libs/guest/xg_sr_save_x86_pv.c
> +++ b/tools/libs/guest/xg_sr_save_x86_pv.c
> @@ -1031,6 +1031,7 @@ static int x86_pv_normalise_page(struct xc_sr_context *ctx, xen_pfn_t type,
>   */
>  static int x86_pv_setup(struct xc_sr_context *ctx)
>  {
> +    xc_interface *xch = ctx->xch;
>      int rc;
>  
>      rc = x86_pv_domain_info(ctx);
> @@ -1049,6 +1050,15 @@ static int x86_pv_setup(struct xc_sr_context *ctx)
>      if ( rc )
>          return rc;
>  
> +    ctx->x86.pv.save.deferred_pages = bitmap_alloc(ctx->save.p2m_size);
> +
> +    if (!ctx->x86.pv.save.deferred_pages)
> +    {
> +        ERROR("Unable to allocate memory for deferred pages");
> +        errno = ENOMEM;
> +        return -1;
> +    }
> +
>      return 0;
>  }
>  
> @@ -1116,9 +1126,35 @@ static int x86_pv_check_vm_state(struct xc_sr_context *ctx)
>      return x86_pv_check_vm_state_p2m_list(ctx);
>  }
>  
> +static int x86_pv_defer_page(struct xc_sr_context *ctx, xen_pfn_t pfn)
> +{
> +    set_bit(pfn, ctx->x86.pv.save.deferred_pages);
> +    ++ctx->x86.pv.save.nr_deferred_pages;
> +
> +    return 0;
> +}
> +
> +static int x86_pv_merge_deferred(const struct xc_sr_context *ctx,
> +                                 unsigned long *bitmap, unsigned long *count)
> +{
> +    bitmap_or(bitmap, ctx->x86.pv.save.deferred_pages, ctx->save.p2m_size);
> +    *count = ctx->x86.pv.save.nr_deferred_pages;
> +
> +    return 0;
> +}
> +
> +static int x86_pv_reset_deferred(struct xc_sr_context *ctx)
> +{
> +    bitmap_clear(ctx->x86.pv.save.deferred_pages, ctx->save.p2m_size);
> +    ctx->x86.pv.save.nr_deferred_pages = 0;
> +
> +    return 0;
> +}
> +
>  static int x86_pv_cleanup(struct xc_sr_context *ctx)
>  {
>      free(ctx->x86.pv.p2m_pfns);
> +    free(ctx->x86.pv.save.deferred_pages);
>  
>      if ( ctx->x86.pv.p2m )
>          munmap(ctx->x86.pv.p2m, ctx->x86.pv.p2m_frames * PAGE_SIZE);
> @@ -1142,6 +1178,9 @@ struct xc_sr_save_ops save_ops_x86_pv =
>      .start_of_checkpoint = x86_pv_start_of_checkpoint,
>      .end_of_checkpoint   = x86_pv_end_of_checkpoint,
>      .check_vm_state      = x86_pv_check_vm_state,
> +    .defer_page          = x86_pv_defer_page,
> +    .merge_deferred      = x86_pv_merge_deferred,
> +    .reset_deferred      = x86_pv_reset_deferred,
>      .cleanup             = x86_pv_cleanup,
>  };
>