On 22.01.2026 13:48, Roger Pau Monné wrote:
> On Mon, Jan 19, 2026 at 02:00:49PM +0100, Jan Beulich wrote:
>> On 15.01.2026 12:18, Roger Pau Monne wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -722,6 +722,15 @@ static void _domain_destroy(struct domain *d)
>>>
>>> XVFREE(d->console);
>>>
>>> + if ( d->pending_scrub )
>>> + {
>>> + BUG_ON(d->creation_finished);
>>> + free_domheap_pages(d->pending_scrub, d->pending_scrub_order);
>>> + d->pending_scrub = NULL;
>>> + d->pending_scrub_order = 0;
>>> + d->pending_scrub_index = 0;
>>> + }
>>
>> Because of the other zeroing wanted (it's not strictly needed, is it?),
>> it may be a little awkward to use FREE_DOMHEAP_PAGES() here. Yet I would
>> still have recommended to avoid its open-coding, if only we had such a
>> wrapper already.
>
> I don't mind introducing a FREE_DOMHEAP_PAGES() wrapper in this same
> patch, if you are OK with it.
I'd be fine with that.
>> Would this better be done earlier, in domain_kill(), to avoid needlessly
>> holding back memory that isn't going to be used by this domain anymore?
>> Would require the spinlock be acquired to guard against a racing
>> stash_allocation(), I suppose. In fact freeing right in
>> domain_unpause_by_systemcontroller() might be yet better (albeit without
>> eliminating the need to do it here or in domain_kill()).
>
> Even with a lock taken moving to domain_kill() would be racy. A rogue
> toolstack could keep trying to issue populate_physmap hypercalls which
> would fail in the assign_pages() call, but it could still leave
> pending pages in d->pending_scrub, as the assign_pages() call happens
> strictly after the scrubbing is done.
As indicated, the freeing here may need to stay. But making an attempt far
earlier may help the system overall.
>>> + /*
>>> + * If there's a pending page to scrub check it satisfies the current
>>> + * request. If it doesn't keep it stashed and return NULL.
>>> + */
>>> + if ( !d->pending_scrub || d->pending_scrub_order != order ||
>>> + (node != NUMA_NO_NODE && node != page_to_nid(d->pending_scrub)) )
>>
>> Ah, and MEMF_exact_node is handled in the caller.
>>
>>> + goto done;
>>> + else
>>> + {
>>> + page = d->pending_scrub;
>>> + *scrub_index = d->pending_scrub_index;
>>> + }
>>> +
>>> + /*
>>> + * The caller now owns the page, clear stashed information. Prevent
>>> + * concurrent usages of get_stashed_allocation() from returning the
>>> same
>>> + * page to different contexts.
>>> + */
>>> + d->pending_scrub_index = 0;
>>> + d->pending_scrub_order = 0;
>>> + d->pending_scrub = NULL;
>>> +
>>> + done:
>>> + rspin_unlock(&d->page_alloc_lock);
>>> +
>>> + return page;
>>> +}
>>
>> Hmm, you free the earlier allocation only in stash_allocation(), i.e. that
>> memory isn't available to fulfill the present request. (I do understand
>> that the freeing there can't be dropped, to deal with possible races
>> caused by the toolstack.)
>
> Since we expect populate_physmap(9 to be executed sequentially by the
> toolstack I would argue it's fine to hold onto that memory.
Here you say "sequentially", just to ...
> Otherwise
> I could possibly free in get_stashed_allocation() when the request
> doesn't match what's stashed. I opted for freeing later in
> stash_allocation() to maybe give time for the other parallel caller to
> finish the scrubbing.
... assume non-sequential behavior here. I guess I'm a little confused.
(Yes, freeing right in get_stashed_allocation() is what I'd expect.)
>> The use of "goto" here also looks a little odd, as it would be easy to get
>> away without. Or else I'd like to ask that the "else" be dropped.
>
> Hm, OK, let me use an unlock + return and also drop the else then. I
> think that's clearer.
I think if() with the condition inverted and a single unlock+return at
the end would be easiest to follow.
Jan
