On Fri, Sep 12, 2025 at 7:01 PM Andrew Cooper <andrew.coop...@citrix.com> 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.zig...@cloud.com>
> > Release-Acked-by: Oleksii Kurochko <oleksii.kuroc...@gmail.com>
>
> Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com>
>
> 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
