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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Tue Mar 11, 2025 at 3:45 PM GMT, 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.
>
> Jan

Why would it have to be arch-specific though? As far as the hypervisor is
concerned, it doesn't seem to be.

Cheers,
Alejandro