On 23.12.2025 09:15, Roger Pau Monne wrote:
> The current logic splits the update of the amount of available memory in
> the system (total_avail_pages) and pending claims into two separately
> locked regions. This leads to a window between counters adjustments where
> the result of total_avail_pages - outstanding_claims doesn't reflect the
> real amount of free memory available, and can return a negative value due
> to total_avail_pages having been updated ahead of outstanding_claims.
>
> Fix by adjusting outstanding_claims and d->outstanding_pages in the same
> place where total_avail_pages is updated. This can possibly lead to the
> pages failing to be assigned to the domain later, after they have already
> been subtracted from the claimed amount. Ultimately this would result in a
> domain losing part of it's claim, but that's better than the current skew
> between total_avail_pages and outstanding_claims.
For the system as a whole - yes. For just the domain rather not. It may be
a little cumbersome, but can't we restore the claim from the error path
after failed assignment? (In fact the need to (optionally) pass a domain
into free_heap_pages() would improve symmetry with alloc_heap_pages().)
> Fixes: 65c9792df600 ("mmu: Introduce XENMEM_claim_pages (subop of memory
> ops)")
> Signed-off-by: Roger Pau Monné <[email protected]>
> ---
> Arguably we could also get rid of domain_adjust_tot_pages() given what it
> currently does, which will be a revert of:
>
> 1c3b9dd61dab xen: centralize accounting for domain tot_pages
>
> Opinions? Should it be done in a separate commit, possibly as a clear
> revert? Maybe it's worth keeping the helper in case we need to add more
> content there, and it's already introduced anyway.
Personally I think we're better off keeping that helper, even if it's now
pretty thin.
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -515,30 +515,6 @@ unsigned long domain_adjust_tot_pages(struct domain *d,
> long pages)
> ASSERT(rspin_is_locked(&d->page_alloc_lock));
> d->tot_pages += 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 ( !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;
> - }
> - spin_unlock(&heap_lock);
> -
> -out:
> return d->tot_pages;
> }
Below here the first comment in domain_set_outstanding_pages() refers to
the code being deleted, and hence imo wants updating, too.
> @@ -1071,6 +1047,26 @@ static struct page_info *alloc_heap_pages(
> total_avail_pages -= request;
> ASSERT(total_avail_pages >= 0);
>
> + if ( d && d->outstanding_pages && !(memflags & MEMF_no_refcount) )
> + {
> + /*
> + * Adjust claims in the same locked region where total_avail_pages is
> + * adjusted, not doing so would lead to a window where the amount of
> + * free memory (avail - claimed) would be incorrect.
> + *
> + * Note that by adjusting the claimed amount here it's possible for
> + * pages to fail to be assigned to the claiming domain while already
> + * having been subtracted from d->outstanding_pages. Such claimed
> + * amount is then lost, as the pages that fail to be assigned to the
> + * domain are freed without replenishing the claim.
> + */
> + unsigned long outstanding = min(outstanding_claims, request);
> +
> + outstanding_claims -= outstanding;
> + BUG_ON(outstanding > d->outstanding_pages);
> + d->outstanding_pages -= outstanding;
> + }
This now happening with the domain alloc lock not held imo also needs at
least mentioning (if not discussing) in the description. Aiui it's safe as
long as all updates of d->outstanding_pages happen with the heap lock
held. Which in turn may want mentioning in a comment next to the field
definition, for (now) being different from e.g. ->tot_pages and
->xenheap_pages.
Jan
