On 8/27/25 7:23 PM, Andrew Cooper wrote:
> Subject wants to be at least tools/libxc, and probably "Use superpages
> where possible on migrate/resume"
>
>
> On 27/08/2025 1:33 pm, 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.
>> 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.
> It's critical to say somewhere that this is applicable to HVM guests.
>
> You've eluded to it, but it's important to state clearly that, for HVM
> guests, PAGE_DATA records contain metadata about GFNs in aligned 4M
> blocks.  This is why we don't even need to buffer a second record.
>
> It's also worth stating that 1G superpages are left for later.
>
>
> CC-ing Oleksii as release manager.  This is a fix for a (mis)feature
> which has been known for a decade (since Xen 4.6), and for which two
> series have been posted but not managed to get in.  Unlike those series,
> this is a very surgical fix that gets the majority of the perf win back,
> without the complexity of trying to guess at 1G pages.
>
> Therefore I'd like to request that it be considered for 4.21 at this
> juncture.
>
Such a significant performance increase is a good reason to include this
in 4.21. We also still have enough time to test it properly.

If there are no objections:Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> Thanks. 
~ Oleksii

>> Signed-off-by: Frediano Ziglio<frediano.ziglio@cloud.com>
>> ---
>>   tools/libs/guest/xg_sr_restore.c | 39 ++++++++++++++++++++++++++++----
>>   1 file changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libs/guest/xg_sr_restore.c b/tools/libs/guest/xg_sr_restore.c
>> index 06231ca826..8dcb1b19c5 100644
>> --- a/tools/libs/guest/xg_sr_restore.c
>> +++ b/tools/libs/guest/xg_sr_restore.c
>> @@ -129,6 +129,8 @@ static int pfn_set_populated(struct xc_sr_context *ctx, xen_pfn_t pfn)
>>       return 0;
>>   }
>>   
>> +#define IS_POWER_OF_2(n) (((n) & ((n) - 1)) == 0)
>> +
>>   /*
>>    * 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
>> @@ -141,6 +143,7 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned int count,
>>       xen_pfn_t *mfns = malloc(count * sizeof(*mfns)),
>>           *pfns = malloc(count * sizeof(*pfns));
>>       unsigned int i, nr_pfns = 0;
>> +    bool contiguous = true;
>>       int rc = -1;
>>   
>>       if ( !mfns || !pfns )
>> @@ -159,18 +162,46 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned int count,
>>               if ( rc )
>>                   goto err;
>>               pfns[nr_pfns] = mfns[nr_pfns] = original_pfns[i];
>> +            if ( pfns[nr_pfns] != pfns[0] + nr_pfns )
>> +                contiguous = false;
>>               ++nr_pfns;
>>           }
>>       }
>>   
>>       if ( nr_pfns )
>>       {
>> -        rc = xc_domain_populate_physmap_exact(
>> -            xch, ctx->domid, nr_pfns, 0, 0, mfns);
>> +        /* try optimizing using larger order */
>> +        rc = -1;
>> +        /*
>> +         * The "nr_pfns <= (1 << 18)" check is mainly for paranoia, it should
>> +         * never happen, the sender would have to send a really large packet.
>> +         */
>> +        if ( contiguous && nr_pfns <= (1 << 18) &&
> This is an arbitrary limit, and to contradict the prior feedback given
> in private, the domain's MAX_ORDER isn't relevant here.  It's the
> toolstack's choice how to lay out the guest in memory.
>
>> +             IS_POWER_OF_2(nr_pfns) && (pfns[0] & (nr_pfns - 1)) == 0 )
>> +        {
>> +            const unsigned int extent_order = __builtin_ffs(nr_pfns) - 1;
> This (non-)loop isn't great.  You'll e.g. use 4k pages for the second 2M
> page of an HVM guest simply because the VGA hole exists in the first.
>
> I think you probably want something like:
>
> int populate_physmap_4k(ctx, nr, mfns);
> int populate_physmap_2M(ctx, nr, mfns);
>
> as simple wrappers around the raw hypercalls including transforming back
> the mfns[] array, and:
>
> int populate_phymap(...);
>
> with logic of the form:
>
>      while ( nr )
>      {
>          if ( nr < 512 ) /* Can't be a superpage */
>          {
>              populate_physmap_4k(ctx, i, mfns);
>              mfns += i;
>              nr -= i;
>              continue;
>          }
>
>          if ( !ALIGNED_2M(mfn) ) /* Populate up until a point that could be a superpage */
>          {
>              while ( !ALIGNED_2M(mfn + i) )
>                  i++;
>              populate_physmap_4k(ctx, i, mfns);
>              mfns += i;
>              nr -= i;
>          }
>
>          if ( nr >= 512 )
>          {
>              for ( i = 1; i < 512; ++i )
>                  if ( mfns[i] != mfns[0] + i )
>                      break;
>              if ( i == 512 )
>                  populate_physmap_2M(ctx, i, mfns);
>              else
>                  populate_physmap_4k(...);
>
>              mfns += i;
>              nr -= i;
>          }
>      }
>
>
>
> Obviously with error handling, and whatever boundary conditions I've got
> wrong.
>
> 2M is the only size that matters (ignoring 1G which we've excluded for
> now), and this form will reconstruct more superpages for the guest than
> trying to do 4M allocations will.
>
> ~Andrew