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 <jbeul...@suse.com>
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
