Re: [PATCH 5/9] xen: introduce assign_pages_nr

[Julien Grall <julien@xxxxxxx>]

Hi,

On 10/06/2021 10:49, Jan Beulich wrote:
> On 07.06.2021 04:43, Penny Zheng wrote:
> > Introduce new interface assign_pages_nr to deal with when page number is
> > not in a power-of-two, which will save the trouble each time user needs
> > to split the size in a power of 2 to use assign_pages.
> First of all I still don't see why in this one special case it is a
> meaningful burden to do the count-to-order conversion in the caller you
> mean to add,
This sort of works for one caller. However, I would expect some more 
user in the future (we use it for Live-Update).
> and hence why we really need this new function (to keep it
> simple, you could even have the caller not break down to arbitrary
> power-of-2 chunks, but simply iterate over all individual [order-0]
> pages).
The function assign_pages() will always use 1U << order (and sadly 1 << 
order). So we would end up to convert the count in multiple order for 
then directly converting back to a number. To me, this sounds rather 
pointless...
There are also a slight benefits to call assign_pages() a single time 
during boot because it will reduce the number of time we need to 
lock/unlock d->page_alloc_lock.
> The more that I'm not happy with the chosen name, despite it
> having been suggested during v1 review. _If_ we needed two functions,
> imo they ought to be named assign_page() (dealing with a single page of
> the given order) and assign_pages(). Backporting confusion could be
> helped by altering the order of parameters, such that the compiler
> would point out that adjustments at call sites are needed.
>
> Irrespective of this a few remarks on the code change itself:
>
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2301,14 +2301,14 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
> >   }
> >   
> >   
> > -int assign_pages(
> > +int assign_pages_nr(
> >       struct domain *d,
> >       struct page_info *pg,
> > -    unsigned int order,
> > +    unsigned int nr_pfns,
> Even leaving the naming aspect of "pfns" aside, I can't see why this
> can't be simply "nr" (of appropriate type, see next remark).
>
> >       unsigned int memflags)
> >   {
> >       int rc = 0;
> > -    unsigned long i;
> > +    unsigned int i;
> This is not an acceptable type change, at least not as long as it's not
> justified at all in the description. While both Arm and x86 will be
> fine this way, the code here is supposed to be generic, and hence would
> better remain generally correct.
I would like to point out the code is already not correct as we are 
using 1U << order or worse 1 << order :).
Cheers,

--
Julien Grall