Re: [PATCH v6 2/7] xen/mm: Allow free pages to cover a claims shortfall

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2026 15:22, Bernhard Kaindl wrote:
> Allow an allocation to proceed when free, unclaimed memory can cover a
> claims shortfall.
> 
> In preparation for NUMA-aware claims, refactor this check into a
> reusable helper so the same logic can be applied to NUMA nodes.
> 
> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Bernhard Kaindl <bernhard.kaindl@xxxxxxxxxx>
> 
> ---
> 
> Changes since v4: Extracted into a dedicated patch and merged the
> formerly separate functions into one, as suggested by Jan Beulich.

I did ask for the splitting out, no question. I can't find anything towards
the functional change though in my v4 replies, and with that being the main
aspect here I think Suggested-by: would credit me for something I didn't do
or say.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -868,6 +868,42 @@ static void check_and_stop_scrub(struct page_info *head)
>      }
>  }
>  
> +/*
> + * Allow the request when unclaimed pages suffice, or when a refcounted
> + * domain's outstanding claims cover the shortfall.

Nit: It's not the domain which is refcounted here (the refcounting there is
something entirely different), but the allocation.

> + */
> +static bool claims_permit_request(const struct domain *d,
> +                                  unsigned long avail_pages,
> +                                  unsigned long competing_claims,
> +                                  unsigned int memflags,
> +                                  unsigned long requested_pages)

I think it is good practice to have the main input parameter(s) first.
That would be "requested_pages" here plus perhaps "memflags". The rest is
auxiliary data to perform the needed calculation.

May I further suggest to drop _pages everywhere?

I'm further not happy about "competing" in the parameter name: This
includes claims for this very domain as well, and if there's no other
claim then there's nothing "competing" at all.

> +{
> +    unsigned long unclaimed_pages;
> +
> +    ASSERT(spin_is_locked(&heap_lock));
> +    ASSERT(avail_pages >= competing_claims);
> +
> +    /* Start from the free pages not already claimed by other domains. */
> +    unclaimed_pages = avail_pages - competing_claims;

This could easily be the initializer of the variable.

Jan