Re: [PATCH] xen/page_alloc: Simplify domain_adjust_tot_pages

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 24, 2025 at 01:27:24PM +0000, Alejandro Vallejo wrote:
> The logic has too many levels of indirection and it's very hard to
> understand it its current form. Split it between the corner case where
> the adjustment is bigger than the current claim and the rest to avoid 5
> auxiliary variables.
> 
> While at it, fix incorrect field name in nearby comment.
> 
> Not a functional change (although by its nature it might not seem
> immediately obvious at first).
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
>  xen/common/page_alloc.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 1bf070c8c5df..4306753f0bd2 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -490,13 +490,11 @@ static long outstanding_claims; /* total outstanding 
> claims by all domains */
>  
>  unsigned long domain_adjust_tot_pages(struct domain *d, long pages)
>  {
> -    long dom_before, dom_after, dom_claimed, sys_before, sys_after;
> -
>      ASSERT(rspin_is_locked(&d->page_alloc_lock));
>      d->tot_pages += pages;
>  
>      /*
> -     * can test d->claimed_pages race-free because it can only change
> +     * 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
>       */
> @@ -504,17 +502,16 @@ unsigned long domain_adjust_tot_pages(struct domain *d, 
> long pages)
>          goto out;

I think you can probably short-circuit the logic below if pages == 0?
(and avoid taking the heap_lock)

>  
>      spin_lock(&heap_lock);
> -    /* adjust domain outstanding pages; may not go negative */
> -    dom_before = d->outstanding_pages;
> -    dom_after = dom_before - pages;
> -    BUG_ON(dom_before < 0);
> -    dom_claimed = dom_after < 0 ? 0 : dom_after;
> -    d->outstanding_pages = dom_claimed;
> -    /* flag accounting bug if system outstanding_claims would go negative */
> -    sys_before = outstanding_claims;
> -    sys_after = sys_before - (dom_before - dom_claimed);
> -    BUG_ON(sys_after < 0);
> -    outstanding_claims = sys_after;
> +    BUG_ON(outstanding_claims < d->outstanding_pages);
> +    if ( pages > 0 && 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;

I wonder if it's intentional for a pages < 0 value to modify
outstanding_claims and d->outstanding_pages, I think those values
should only be set from domain_set_outstanding_pages().
domain_adjust_tot_pages() should only decrease the value, but never
increase either outstanding_claims or d->outstanding_pages.

At best the behavior is inconsistent, because once
d->outstanding_pages reaches 0 there will be no further modification
from domain_adjust_tot_pages().

I think it would be best if outstanding_claims and
d->outstanding_pages was only modified if pages > 0.

Thanks, Roger.