Non-functional change to make consuming claims more concise:

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.

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
Co-authored-by: Roger Pau Monné <roger.pau@citrix.com>

---
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)

Previous reviews
----------------

Review with the requested changes to the commit message:
- https://patchew.org/Xen/cover.1757261045.git.bernhard.kaindl@cloud.com/15ae395c6933e74da0cdd8f9d71d349a7bfad3f3.1757261045.git.bernhard.kaindl@cloud.com/

Obsoleted review:
- https://patchew.org/Xen/cover.1755341947.git.bernhard.kaindl@cloud.com/5f417fea5ca8e4da0d4b9679103c3eff4bc92900.1755341947.git.bernhard.kaindl@cloud.com/
---
 xen/common/page_alloc.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 1f67b88a89..ae2a560e0a 100644
--- 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. */
 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.
      */
     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 */
+    adjustment = min_t(unsigned long, d->outstanding_pages, pages);
+    d->outstanding_pages -= adjustment;
+    outstanding_claims -= adjustment;
     spin_unlock(&heap_lock);
 
 out:
-- 
2.34.1

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@cloud.com>
> Co-authored-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> ---
> 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