Re: [PATCH v2] xen/page_alloc: Simplify domain_adjust_tot_pages

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Mar 11, 2025 at 04:45:04PM +0100, Jan Beulich wrote:
> On 11.03.2025 16:42, Roger Pau Monné wrote:
> > On Tue, Mar 11, 2025 at 02:53:04PM +0000, Alejandro Vallejo wrote:
> >> On Tue Mar 11, 2025 at 12:46 PM GMT, Roger Pau Monné wrote:
> >>> On Tue, Mar 04, 2025 at 11:10:00AM +0000, Alejandro Vallejo wrote:
> >>>> The logic has too many levels of indirection and it's very hard to
> >>>> understand it its current form. Split it between the corner case where
> >>>> the adjustment is bigger than the current claim and the rest to avoid 5
> >>>> auxiliary variables.
> >>>>
> >>>> Add a functional change to prevent negative adjustments from
> >>>> re-increasing the claim. This has the nice side effect of avoiding
> >>>> taking the heap lock here on every free.
> >>>>
> >>>> While at it, fix incorrect field name in nearby comment.
> >>>>
> >>>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> >>>
> >>> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>
> >> Thanks.
> >>
> >>> I think it would be nice to also ensure that once the domain is
> >>> finished building (maybe when it's unpaused for the first
> >>> time?) d->outstanding_pages is set to 0.  IMO the claim system was
> >>> designed to avoid races during domain building, and shouldn't be used
> >>> once the domain is already running.
> >>>
> >>> Thanks, Roger.
> >>
> >> As a matter of implementation that's already the case by toolstack being 
> >> "nice"
> >> and unconditionally clearing claims after populating the physmap.
> > 
> > I see.  Another option would be to refuse the unpause a domain if it
> > still has pending claims.  However I don't know how that will work out
> > with all possible toolstacks.
> > 
> >> However, I agree the hypervisor should do it on its own. I didn't find a
> >> suitable place for it. 
> > 
> > You could do it in arch_domain_creation_finished().
> 
> Except that better wouldn't be arch-specific.

Sorry, I should better have said: you could do it at the same place
where arch_domain_creation_finished() gets called in
domain_unpause_by_systemcontroller().

Thanks, Roger.