Re: [PATCH] xen/mm: move adjustment of claimed pages counters on allocation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.12.2025 18:55, Roger Pau Monné wrote:
> On Tue, Dec 23, 2025 at 11:59:54AM +0100, Jan Beulich wrote:
>> 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().)
> 
> Passing a domain parameter to free_heap_pages() is not that much of an
> issue.  The problem with restoring the claim value on failure to
> assign is the corner cases.  For example consider an allocation that
> depletes the existing claim, allocating more than what was left to be
> claimed.  Restoring the previous claim value on failure to assign to
> the domain would be tricky.  It would require returning the consumed
> claim from alloc_heap_pages(), so that alloc_domheap_pages() could
> restore it on failure to assign.
> 
> However, I was looking at the possible failure causes of
> assign_pages() and I'm not sure there's much point in attempting to
> restore the claimed amount.  Current cases where assign_pages() will
> fail:
> 
>  - Domain is dying: keeping the claim is irrelevant, the domain is
>    dying anyway.
> 
>  - tot_pages > max_pages: inconsistent domain state, and a claim
>    should never be bigger than max_pages.
> 
>  - tot_pages + alloc > max_pages: only possible if alloc is using
>    claim pages plus unclaimed ones, as the claim cannot be bigger than
>    max_pages.  Such alloc is doomed to fail anyway, and would point at
>    the claim value being incorrectly set.
> 
>  - tot_pages + alloc < alloc: overflow of tot_pages, should never
>    happen with claimed pages as tot_pages <= max_pages, and claim <=
>    max_pages.
> 
> However that only covers current code in assign_pages(), there's no
> guarantee that future code might introduce new failure cases.
> 
> Having said all that, I have a prototype that restores the claimed
> amount that I could send to the list.  It involves adding two extra
> parameters to free_heap_pages(): the domain and the claim amount to
> restore.  It's not super-nice, but I was expecting it to be worse.

With the justification above I'd be okay with the claim not being
restored upon failure; the extra logic could then be added if and when
an error case appears which would make it desirable to restore the
claim.

Jan