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

[Julien Grall <julien@xxxxxxx>]

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