Re: [PATCH v6 4/7] xen/mm: Split outstanding claims into global and node totals

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2026 15:22, Bernhard Kaindl wrote:
> Replace d->outstanding_pages with d->global_claims and add
> d->node_claims as the aggregate of the domain's node-specific claims.
> 
> Keep the allocator hot path efficient and report the combined claims
> state using the two new fields.

The truly new field (node_claims) is in fact dead code (as per Misra's
definition of the term). It therefore doesn't feel quite right that it
is being introduced without (really) being used.

> @@ -552,8 +552,7 @@ int domain_set_outstanding_pages(struct domain *d, 
> unsigned long pages)
>      /*
>       * Two locks are needed here:
>       *  - d->page_alloc_lock: protects accesses to d->{tot,max,extra}_pages.
> -     *  - heap_lock: protects accesses to d->outstanding_pages, 
> total_avail_pages
> -     *    and outstanding_claims.
> +     *  - heap_lock: Protects accesses to the claims and avail_pages state.
>       */
>      nrspin_lock(&d->page_alloc_lock);
>      spin_lock(&heap_lock);

By removing the use of d-> from the bullet point, you make the result appear
to all be only global state. Imo the fields still wants mentioning like it
was before.

> @@ -561,13 +560,13 @@ int domain_set_outstanding_pages(struct domain *d, 
> unsigned long pages)
>      /* pages==0 means "unset" the claim. */
>      if ( pages == 0 )
>      {
> -        deduct_global_claims(d, d->outstanding_pages);
> +        deduct_global_claims(d, d->global_claims);
>          ret = 0;
>          goto out;
>      }
>  
> -    /* only one active claim per domain please */
> -    if ( d->outstanding_pages )
> +    /* Reject updating global claims and we can't update node claims */
> +    if ( d->global_claims || d->node_claims )
>      {
>          ret = -EINVAL;
>          goto out;

Is there anything wrong with the original comment (apart from style)?
Especially the new "we can't" feels misleading - if we indeed can't,
this is something that could be fixed. Isn't more like we may not
fiddle with node claims here? Plus mentioning node claims when they
aren't a thing yet is somewhat odd, too. Remember: Patches in a series
may go if with arbitrarily large gaps in between.

> @@ -891,7 +890,7 @@ static bool claims_permit_request(const struct domain *d,
>                                    unsigned int memflags,
>                                    unsigned long requested_pages)
>  {
> -    unsigned long unclaimed_pages;
> +    unsigned long unclaimed_pages, applicable_claims;
>  
>      ASSERT(spin_is_locked(&heap_lock));
>      ASSERT(avail_pages >= competing_claims);
> @@ -910,11 +909,13 @@ static bool claims_permit_request(const struct domain 
> *d,
>      if ( !d || (memflags & MEMF_no_refcount) )
>          return false;
>  
> +    applicable_claims = d->global_claims;
> +
>      /*
>       * Allow the request to proceed when combination of unclaimed pages and 
> the
>       * claims held by the domain cover the shortfall for the requested_pages.
>       */
> -    return requested_pages <= unclaimed_pages + d->outstanding_pages;
> +    return requested_pages <= unclaimed_pages + applicable_claims;
>  }

I don't follow what use these two hunks are here.

> @@ -1112,18 +1113,16 @@ static struct page_info *alloc_heap_pages(
>      total_avail_pages -= request;
>      ASSERT(total_avail_pages >= 0);
>  
> -    if ( d && d->outstanding_pages && !(memflags & MEMF_no_refcount) )
> +    if ( d && d->global_claims && !(memflags & MEMF_no_refcount) )
>      {
>          /*
>           * Adjust claims in the same locked region where total_avail_pages is
>           * adjusted, not doing so would lead to a window where the amount of
>           * free memory (avail - claimed) would be incorrect.
>           *
> -         * Note that by adjusting the claimed amount here it's possible for
> -         * pages to fail to be assigned to the claiming domain while already
> -         * having been subtracted from d->outstanding_pages.  Such claimed
> -         * amount is then lost, as the pages that fail to be assigned to the
> -         * domain are freed without replenishing the claim.  This is fine 
> given
> +         * Note, after redeeming claims for the allocation here, 
> assign_pages()
> +         * could fail. The domain looses The redeemed claims as the not 
> assigned

Nit: ... loses the ...

> +         * pages are freed without replenishing the claim.  This is fine 
> given
>           * claims are only to be used during physmap population as part of
>           * domain build, and any failure in assign_pages() there will result 
> in
>           * the domain being destroyed before creation is finished.  Losing 
> part

I'm not convinced this comment needs fiddling with. There's nothing obviously
wrong, and I don't see an obvious improvement from the changes. 

Jan