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

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

On Wed Mar 12, 2025 at 8:21 AM GMT, Jan Beulich wrote:
> On 11.03.2025 19:35, Alejandro Vallejo wrote:
> > 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.
> > 
> > Why would it have to be arch-specific though? As far as the hypervisor is
> > concerned, it doesn't seem to be.
>
> Together with Roger's earlier clarification on his original remark, I fear
> I don't understand the question: I asked that it not be arch-specific. And
> Roger clarified that he also didn't mean it to be.
>
> Jan

Bah, I misread you (and my IMAP server annoyingly decided enough was enough and
declared Roger's answer was one too many and deferred sending it until much
later).

Too much going on, too little attention. Apologies for the noise.

I'll send a patch at some point with that adjustment.

Cheers,
Alejandro