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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 01/07/2021 09:26, Jan Beulich wrote:
> On 30.06.2021 20:29, Julien Grall wrote:
> > 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.
> Well, all of this is why I did add ...
>
> > > 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.
> ... this. 
Oh, it wasn't entirely clear whether you were objecting of offering the 
possibility to pass a number of pages rather than an order.
> Not sure whether you not commenting on it means you agree
> with the proposal.
Yes I am happy with your proposal.

> > > > --- 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 :).
> Indeed there are improvements (towards being consistent) to be made. But
> this is not an excuse to make things worse here. At least one of the two
> loops already properly uses 1ul; sadly that's only debugging code. And
> of course something like domain_tot_pages() (and the underlying field)
> dealing with "unsigned int" doesn't help consistency either. As it stands
> we're limiting ourselves to 8Tb VMs, as it seems, and for no good reason.
I actually have a patch in my queue that hardens assign_pages(). I will 
aim to send it later today.
Cheers,

--
Julien Grall