Re: [PATCH v2 3/3] xen/heap: pass order to free_heap_pages() in heap init

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 18/07/2022 10:43, Jan Beulich wrote:
> On 15.07.2022 19:03, Julien Grall wrote:
> > @@ -1806,8 +1806,22 @@ static void _init_heap_pages(const struct page_info *pg,
> >   
> >       while ( s < e )
> >       {
> > -        free_heap_pages(mfn_to_page(_mfn(s)), 0, need_scrub);
> > -        s += 1UL;
> > +        /*
> > +         * For s == 0, we simply use the largest increment by checking the
> > +         * MSB of the region size. For s != 0, we also need to ensure that the
> > +         * chunk is properly sized to end at power-of-two alignment. We do this
> > +         * by checking the LSB of the start address and use its index as
> > +         * the increment. Both cases need to be guarded by MAX_ORDER.
> s/guarded/bounded/ ?
>
> > +         * Note that the value of ffsl() and flsl() starts from 1 so we need
> > +         * to decrement it by 1.
> > +         */
> > +        int inc_order = min(MAX_ORDER, flsl(e - s) - 1);
> unsigned int, since ...
MAX_ORDER, flsl(), ffsl() are both returning signed value. So inc_order 
should be 'int' to avoid any compilation issue.
> > +        if ( s )
> > +            inc_order = min(inc_order, ffsl(s) - 1);
> > +        free_heap_pages(mfn_to_page(_mfn(s)), inc_order, need_scrub);
> ... that's what the function parameter says, and the value also can go
> negative?
AFAICT, none of the values are negative. But per above, if we use min() 
then the local variable should be 'int'. The two possible alternatives are:
  1) Use min_t()
  2) Update MAX_ORDER, flsl(), ffsl() to return an unsigned value

The ideal would be #2 but it will require at least an extra patch and 
effort. If we use #1, then they use may become stale if we implement #2.
So I would prefer to keep min(). That said, I would be open to use 
min_t() if you strongly prefer it.
> Preferably with these adjustments
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
Thanks!

Cheers,

--
Julien Grall