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 <[email protected]> > Co-authored-by: Roger Pau Monné <[email protected]> > > --- > 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
