Re: [PATCH] xen/page_alloc: Much more concise domain_adjust_tot_pages()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.12.2025 11:02, Bernhard Kaindl wrote:
> Non-functional change to make consuming claims more concise:

"Much more concise" in the title suggests a significant (i.e. functional)
change, contrary to what is being said here.

> When we limit the consumption of claims to the remaining claims
> of the domain, we can make the code more concise by limiting the
> adjustment to it instead of carrying a special case for it.

This wording in turn suggests a functional change when, afaict, there is
none.

> Signed-off-by: Bernhard Kaindl <bernhard.kaindl@xxxxxxxxx>
> Co-authored-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> ---
> Changes
> - Use min_t(unsigned long, a, b) as the tool of the trade (Roger Pau Monné)
> - Reviewed by Andrew Cooper and Roger Pau Monné(Excluding comments, commit 
> message)
> - Regression-tested and included as part of te NUMA work for XenServer 9
> - Improved comments and the commit message (non-functional change, comment 
> cleanup)

If there was a previous version, why does this submission not have a
version number > 1? Having looked back - what was wrong with the
original title?

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -510,8 +510,11 @@ static unsigned long avail_heap_pages(
>      return free_pages;
>  }
>  
> +/* Adjust the tot_pages and remaining outstanding claims of the domain. */

Nit: Why once a field name and once a verbal description?

>  unsigned long domain_adjust_tot_pages(struct domain *d, long pages)
>  {
> +    unsigned long adjustment;
> +
>      ASSERT(rspin_is_locked(&d->page_alloc_lock));
>      d->tot_pages += pages;
>  
> @@ -519,23 +522,19 @@ unsigned long domain_adjust_tot_pages(struct domain *d, 
> long pages)
>       * can test d->outstanding_pages race-free because it can only change
>       * if d->page_alloc_lock and heap_lock are both held, see also
>       * domain_set_outstanding_pages below
> +     *
> +     * skip claims adjustment when the domain has no outstanding claims
> +     * or we unassigned pages from it.

Nit: Comment style.

>       */
>      if ( !d->outstanding_pages || pages <= 0 )
>          goto out;
>  
>      spin_lock(&heap_lock);
>      BUG_ON(outstanding_claims < d->outstanding_pages);
> -    if ( d->outstanding_pages < pages )
> -    {
> -        /* `pages` exceeds the domain's outstanding count. Zero it out. */
> -        outstanding_claims -= d->outstanding_pages;
> -        d->outstanding_pages = 0;
> -    }
> -    else
> -    {
> -        outstanding_claims -= pages;
> -        d->outstanding_pages -= pages;
> -    }
> +    /* consume claims until the domain's outstanding_claims are exhausted */

Again: Comment style.

> +    adjustment = min_t(unsigned long, d->outstanding_pages, pages);

I may have expressed before that any use of min_t() is worrying to a certain
degree, due to the involved casting. The use here may well be appropriate,
but I'd expect a word to be said towards this in the description then. After
all alternatives exist (and I even pointed out one during v3 review) ...

Jan