Re: [PATCH v3] tools/libs: Use superpages where possible on migrate/resume

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Fri, Sep 12, 2025 at 7:01 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 12/09/2025 10:57 am, Frediano Ziglio wrote:
> > Try to allocate larger order pages.
> > With some test memory program stressing TLB (many small random
> > memory accesses) you can get 15% performance improves.

Checker is suggesting "improvements" instead of "improves" here. Can
you also update this ?

> > On the first memory iteration the sender is currently sending
> > memory in 4mb aligned chunks which allows the receiver to
> > allocate most pages as 2mb superpages instead of single 4kb pages.
> > This works even for HVM where the first 2mb contains some holes.
> > This change does not handle 1gb superpages as this will require
> > change in the protocol to preallocate space.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> > Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> Thanks, this is far easier to follow.  A couple of minor things.
>
> > ---
> > Changes since v1:
> > - updated commit message and subject;
> > - change the implementation detecting possible 2mb pages inside
> >   the packet sent allowing more 2mb superpages.
> >
> > Changes since v2:
> > - change implementation simplifying detecting and allocations
> >   of 2mb pages.
> > ---
> >  tools/libs/guest/xg_sr_restore.c | 45 +++++++++++++++++++++++++++++---
> >  1 file changed, 42 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/libs/guest/xg_sr_restore.c 
> > b/tools/libs/guest/xg_sr_restore.c
> > index 06231ca826..ea5a137612 100644
> > --- a/tools/libs/guest/xg_sr_restore.c
> > +++ b/tools/libs/guest/xg_sr_restore.c
> > @@ -129,6 +129,30 @@ static int pfn_set_populated(struct xc_sr_context 
> > *ctx, xen_pfn_t pfn)
> >      return 0;
> >  }
> >
> > +#if defined(__i386__) || defined(__x86_64__)
> > +/* Order of the smallest superpage */
> > +#define SMALL_SUPERPAGE_ORDER 9
> > +#else
> > +#error Define SMALL_SUPERPAGE_ORDER for this platform
> > +#endif
> > +
> > +static bool populate_small_superpage(struct xc_sr_context *ctx, xen_pfn_t 
> > pfn)
>
> I know the terminology is terrible (this work was what prompted some of
> my clean-up attempts in Xen).
>
> I think we want to s/pfn/gfn/ all across this function.
>
> > +{
> > +    xen_pfn_t mfn = pfn;
> > +
> > +    if ( xc_domain_populate_physmap_exact(
> > +         ctx->xch, ctx->domid, 1, SMALL_SUPERPAGE_ORDER, 0, &mfn) )
>
> This needs a comment.
>
> /* XENMEM_populate_physmap has no coherent error semantics.  Assume a
> failure here is ENOMEM, and fall back to allocating small pages. */
>
> (Yes, the physmap hypercalls are insane.  The only error feedback is "I
> completed this many before something went wrong", and libxenctrl chooses
> EBUSY for want of anything better.)
>
> > +        return false;
> > +
> > +    if ( mfn == INVALID_MFN )
> > +        return false;
> > +
> > +    for ( size_t i = 0; i < (1 << SMALL_SUPERPAGE_ORDER); ++i )
> > +        ctx->restore.ops.set_gfn(ctx, pfn + i, mfn + i);
> > +
> > +    return true;
> > +}
> > +
> >  /*
> >   * Given a set of pfns, obtain memory from Xen to fill the physmap for the
> >   * unpopulated subset.  If types is NULL, no page type checking is 
> > performed
> > @@ -142,6 +166,9 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned 
> > int count,
> >          *pfns = malloc(count * sizeof(*pfns));
> >      unsigned int i, nr_pfns = 0;
> >      int rc = -1;
> > +    xen_pfn_t prev = 0;
> > +    unsigned num_contiguous = 0;
> > +    xen_pfn_t mask = ~((~(xen_pfn_t)0) << SMALL_SUPERPAGE_ORDER);
>
> (1ULL << SMALL_SUPERPAGE_ORDER) - 1; is the more normal way of writing this.
>
> >
> >      if ( !mfns || !pfns )
> >      {
> > @@ -152,14 +179,26 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned 
> > int count,
> >
> >      for ( i = 0; i < count; ++i )
> >      {
> > +        xen_pfn_t pfn = original_pfns[i];
> > +
> >          if ( (!types || page_type_to_populate(types[i])) &&
> > -             !pfn_is_populated(ctx, original_pfns[i]) )
> > +             !pfn_is_populated(ctx, pfn) )
> >          {
> > -            rc = pfn_set_populated(ctx, original_pfns[i]);
> > +            rc = pfn_set_populated(ctx, pfn);
> >              if ( rc )
> >                  goto err;
> > -            pfns[nr_pfns] = mfns[nr_pfns] = original_pfns[i];
> > +            pfns[nr_pfns] = mfns[nr_pfns] = pfn;
> >              ++nr_pfns;
>
> /* For x86 HVM guests in the first pass, PAGE_DATA records contain
> metadata about 4M aligned chunks of GFN space.  Reconstruct 2M
> superpages where possible. */
>
> I'm happy to fix these all on commit, if you're happy?
>

Yes, fine with all changes.
> ~Andrew
>

Frediano