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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.07.2022 12:24, Julien Grall wrote:
> 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.

I'm not sure #2 is a good option - we'd deviate from conventions used
elsewhere on what flsl() and ffsl() return. And constants would imo
best remain suffix-less unless a suffix is very relevant to have (for
MAX_ORDER, which isn't at risk of being subject to ~ or -, it may be
warranted).

3)
        unsigned int inc_order = min(MAX_ORDER, flsl(e - s) - 1);

        if ( s )
            inc_order = min(inc_order, ffsl(s) - 1U);

No compilation issues to expect here, afaict.

> So I would prefer to keep min(). That said, I would be open to use 
> min_t() if you strongly prefer it.

I consider max_t() / min_t() dangerous, so I'd like to limit their use
to cases where no good alternatives exist.

Jan