Re: [PATCH v3 4/7] xen/page_alloc: Add staking a NUMA node claim for a domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.09.2025 18:15, Bernhard Kaindl wrote:
> Update domain_set_outstanding_pages() to domain_claim_pages() for
> staking claims for domains on NUMA nodes:
> 
> domain_claim_pages() is a handler for claiming pages, where its former
> name suggested that it just sets the domain's outstanding claims.
> 
> Actually, three different code locations do perform just this task:
> 
> Fix this using a helper to avoid repeating yourself (an anti-pattern)
> for just only updating the domain's outstanding pages is added as well:
> 
> It removes the need to repeat the same sequence of operations at three
> diffent places and helps to have a single location for adding multi-node
> claims. It also makes the code much shorter and easier to follow.
> 
> Fix the meaning of the claims argument of domain_claim_pages()
> for NUMA-node claims:
> 
> - For NUMA-node claims, we need to claim defined amounts of memory
>   on different NUMA nodes. Previously, the argument was a "reservation"
>   and the claim was made on the difference between d->tot_pages and
>   the reservations. Of course, the argument needed to be > d->tot_pages.
> 
>   This interacs badly with NUMA claims:
>   NUMA node claims are not related to potentially already allocated
>   memory and reducing the claim by already allocated memory would
>   not work in case d->tot_pages already has some amount of pages.
> 
> - Fix this by simply claiming the given amount of pages.
> 
> - Update the legacy caller of domain_claim_pages() accordingly by
>   moving the reduction of the claim by d->tot_pages to it:
> 
>   No change for the users of the legacy hypercall, and a usable
>   interface for staking NUMA claims.
> 
> Signed-off-by: Bernhard Kaindl <bernhard.kaindl@xxxxxxxxx>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>

This looks the wrong way round, and then I expect a From: is also missing.

> ---
> Changes in v3:
> 
> - Renamed domain_set_outstanding_pages() and add check from review.
> - Reorganized v3, v4 and v5 as per review to avoid non-functional
>   changes:

What's v3, v4, and v5 here (when we're only at v3)?

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1682,7 +1682,20 @@ long do_memory_op(unsigned long cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          rc = xsm_claim_pages(XSM_PRIV, d);
>  
>          if ( !rc )
> -            rc = domain_set_outstanding_pages(d, reservation.nr_extents);
> +        {
> +            unsigned long new_claim = reservation.nr_extents;
> +
> +            /*
> +             * For backwards compatibility, keep the meaning of nr_extents:
> +             * it is the target number of pages for the domain.
> +             * In case memory for the domain was allocated before, we must
> +             * substract the already allocated pages from the reservation.
> +             */
> +            if ( new_claim )
> +                new_claim -= domain_tot_pages(d);

This is now racy (and hence a functional change): Without holding the heap
lock, a domain's total pages can change behind you back.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -492,6 +492,30 @@ DEFINE_PER_NODE(unsigned long, avail_pages);
>  
>  static DEFINE_SPINLOCK(heap_lock);
>  static long outstanding_claims; /* total outstanding claims by all domains */
> +DECLARE_PER_NODE(long, outstanding_claims);
> +DEFINE_PER_NODE(long, outstanding_claims);

See comment on the earlier patch.

> +#define domain_has_node_claim(d) (d->claim_node != NUMA_NO_NODE)
> +
> +static inline bool insufficient_memory(unsigned long request, nodeid_t node)

Except in special cases, no inline please for static functions in .c files.

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -405,6 +405,7 @@ struct domain
>      unsigned int     outstanding_pages; /* pages claimed but not possessed */
>      unsigned int     max_pages;         /* maximum value for 
> domain_tot_pages() */
>      unsigned int     extra_pages;       /* pages not included in 
> domain_tot_pages() */
> +    nodeid_t         claim_node;        /* NUMA_NO_NODE for host-wide claims 
> */

I don't quite understand the purpose of this field: It looks to be a
hidden parameter to domain_adjust_outstanding_claim(), yet then why isn't
is a real one?

As I'm also having a hard time following the description, I fear I have to
stay away from making further comments (on the main part of the code
changes), until I understand better what's (intended to be) going on here.

Jan