Re: [PATCH] xen/page_alloc: Simplify domain_adjust_tot_pages

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

On Wed Feb 26, 2025 at 2:02 PM GMT, Jan Beulich wrote:
> On 24.02.2025 14:27, Alejandro Vallejo wrote:
> > @@ -504,17 +502,16 @@ unsigned long domain_adjust_tot_pages(struct domain 
> > *d, long pages)
> >          goto out;
> >  
> >      spin_lock(&heap_lock);
> > -    /* adjust domain outstanding pages; may not go negative */
> > -    dom_before = d->outstanding_pages;
> > -    dom_after = dom_before - pages;
> > -    BUG_ON(dom_before < 0);
> > -    dom_claimed = dom_after < 0 ? 0 : dom_after;
> > -    d->outstanding_pages = dom_claimed;
> > -    /* flag accounting bug if system outstanding_claims would go negative 
> > */
> > -    sys_before = outstanding_claims;
> > -    sys_after = sys_before - (dom_before - dom_claimed);
> > -    BUG_ON(sys_after < 0);
> > -    outstanding_claims = sys_after;
> > +    BUG_ON(outstanding_claims < d->outstanding_pages);
> > +    if ( pages > 0 && d->outstanding_pages < pages )
>
> The lhs isn't needed, is it? d->outstanding_pages is an unsigned quantity,
> after all. Else dropping the earlier of the two BUG_ON() wouldn't be quite
> right.

d->outstanding pages is unsigned, but pages isn't.

It was originally like that, but I then got concerned about 32bit machines,
where you'd be comparing a signed and an unsigned integer of the same
not-very-large width. That seems like dangerous terrains if the unsigned number
grows large enough.

TL;DR: It's there for clarity and paranoia. Even if the overflowing into bit 31
would be rare in such a system.

>
> > +    {
> > +        /* `pages` exceeds the domain's outstanding count. Zero it out. */
> > +        outstanding_claims -= d->outstanding_pages;
> > +        d->outstanding_pages = 0;
> > +    } else {
>
> Nit: Braces on their own lines please.

Ack.

>
> In any event - yes, this reads quite a bit easier after the adjustment.
>
> With the adjustments (happy to make while committing, so long as you agree)
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks. I'd probably like to hold off and send a v2 if you're fine with the
adjustment I answered Roger with (returning ealy on pages <= 0, so claims are
never increased on free).

>
> Jan
>

Cheers,
Alejandro