Re: [PATCH v2 1/7] xen/page_alloc: Simplify domain_adjust_tot_pages for future changes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.08.2025 13:19, Bernhard Kaindl wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -510,8 +510,14 @@ static unsigned long avail_heap_pages(
>      return free_pages;
>  }
>  
> +/*
> + * Update the total number of pages and outstanding claims of a domain.
> + * - When pages were freed, we do not increase outstanding claims.
> + */

If already you add such a comment, please have it be complete: There's
also an update to the global "outstanding_claims" in here.

>  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 +525,22 @@ 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
> +     *
> +     * If the domain has no outstanding claims (or we freed pages instead),
> +     * we don't update outstanding claims and skip the claims adjustment.
>       */
>      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;
> -    }
> +    /*
> +     * Reduce claims by outstanding claims or pages (whichever is smaller):
> +     * If allocated > outstanding, reduce the claims only by outstanding 
> pages.
> +     */
> +    adjustment = min(d->outstanding_pages, (unsigned int)pages);

This would be all fine if there wasn't the cast. It's only a latent problem,
yes, but I think we still would better avoid introducing such. Imo this wants
to be

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

or the equivalent

    adjustment = min(d->outstanding_pages + 0UL, pages + 0UL);

(personally I'd prefer the latter despite its odd look, for not involving
any casts).

Jan