Re: [PATCH] xen/page_alloc: Remove dead code in alloc_domheap_pages()

[Julien Grall <julien@xen.org>]

Hi Jan,

On 07/04/2021 10:25, Jan Beulich wrote:
> On 06.04.2021 21:22, Julien Grall wrote:
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -457,6 +457,12 @@ static long total_avail_pages;
>>   static DEFINE_SPINLOCK(heap_lock);
>>   static long outstanding_claims; /* total outstanding claims by all domains */
>>   
>> +static void __init __maybe_unused build_assertions(void)
>> +{
>> +    /* Zone 0 is reserved for Xen, so we at least need two zones to function.*/
>> +    BUILD_BUG_ON(NR_ZONES < 2);
>> +}
> 
> With a couple of transformations this could also be
> 
>      BUILD_BUG_ON(PADDR_BITS <= PAGE_SHIFT);
> 
> i.e. you're checking that the architecture allows for at least two
> pages to be addressable. Is this really a useful thing to check?

I saw it, but I was concerned that someone may modify the definition of 
NR_ZONES without looking at the rest of the code base.

Anyway, I guess that the ASSERT(zone_hi != 0) in the code should be 
sufficient to catch such problem. So I will drop it.

> 
> Irrespective of the usefulness, if this is to be kept I think the
> function wants to live at the end of the source file, like the
> majority of other files have it (another consistent place could
> be at the top of the file, after all #include-s, as can be found
> in two other cases).
> 
>> @@ -2340,8 +2346,9 @@ struct page_info *alloc_domheap_pages(
>>   
>>       bits = domain_clamp_alloc_bitsize(memflags & MEMF_no_owner ? NULL : d,
>>                                         bits ? : (BITS_PER_LONG+PAGE_SHIFT));
>> -    if ( (zone_hi = min_t(unsigned int, bits_to_zone(bits), zone_hi)) == 0 )
>> -        return NULL;
>> +
>> +    zone_hi = min_t(unsigned int, bits_to_zone(bits), zone_hi);
>> +    ASSERT(zone_hi != 0);
> 
> With the function above preferably dropped or at least moved,
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> I'd like to point out though that I think this would be a good
> opportunity to eliminate the use of min_t() here, by changing
> bits_to_zone()'s 1 to 1u. But I suppose you again would prefer
> to not make this extra change right here, despite it being
> somewhat related to bits_to_zone() only ever returning positive
> values.

In general, I am in not in favor to modify unrelated to code (e.g. 
coding style) or things that deserved a more than a one-line explanation 
in the commit message. This is not the case here, so I will respin it 
and switch from min_t to min.

Cheers,

-- 
Julien Grall