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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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@xxxxxxxx>

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.

Jan