Hi Jan
> -----Original Message-----
> From: Jan Beulich <[email protected]>
> Sent: Monday, July 19, 2021 4:41 PM
> To: Penny Zheng <[email protected]>; [email protected]
> Cc: Bertrand Marquis <[email protected]>; Wei Chen
> <[email protected]>; nd <[email protected]>; [email protected];
> [email protected]
> Subject: Re: [PATCH V3 07/10] xen: re-define assign_pages and introduce
> assign_page
>
> On 15.07.2021 07:18, Penny Zheng wrote:
> > In order to deal with the trouble of count-to-order conversion when
> > page number is not in a power-of-two, this commit re-define
> > assign_pages for nr pages and assign_page for original page with a single
> order.
> >
> > Backporting confusion could be helped by altering the order of
> > assign_pages parameters, such that the compiler would point out that
> > adjustments at call sites are needed.
>
> Back on the initial form of this patch Julien said:
>
> "However, I think I would prefer if we introduce a new interface (maybe
> assign_pages_nr()) rather than change the meaning of the field. This is for
> two
> reasons:
> 1) We limit the risk to make mistake when backporting a patch touch
> assign_pages().
> 2) Adding (1UL << order) for pretty much all the caller is not nice."
>
> 1) is taken care of here anyway (albeit see the remark below), and of the
> callers only some would really need "1UL <<" added (others could simply
> convert their literal 0 to literal 1). Julien - do you still think 2) is
> pretty
> important to avoid at the, overall, 2 places that would be left?
>
> > --- a/xen/arch/x86/pv/dom0_build.c
> > +++ b/xen/arch/x86/pv/dom0_build.c
> > @@ -556,7 +556,7 @@ int __init dom0_construct_pv(struct domain *d,
> > else
> > {
> > while ( count-- )
> > - if ( assign_pages(d, mfn_to_page(_mfn(mfn++)), 0, 0) )
> > + if ( assign_page(d, mfn_to_page(_mfn(mfn++)), 0, 0) )
>
> I think in all cases where order-0 pages get passed, you'd rather want to call
> assign_pages() directly (if, as per above, we'll keep both functions in the
> first
> place).
>
Sure. I'll use literal 1, it's more reasonable to me also.
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2283,8 +2283,8 @@ void init_domheap_pages(paddr_t ps, paddr_t pe)
> >
> > int assign_pages(
> > struct domain *d,
> > + unsigned long nr,
> > struct page_info *pg,
> > - unsigned int order,
> > unsigned int memflags)
> > {
>
> I'm afraid I consider putting "nr" ahead of "pg" unusual (considering the
> rest of
> our code base). How about
>
> int assign_pages(
> struct page_info *pg,
> unsigned long nr,
> struct domain *d,
> unsigned int memflags)
> {
>
> ?
>
Sure. Thx for reconstructing.
> > @@ -2354,6 +2354,11 @@ int assign_pages(
> > return rc;
> > }
> >
> > +int assign_page(struct domain *d, struct page_info *pg, unsigned int order,
> > + unsigned int memflags) {
> > + return assign_pages(d, (1UL << order), pg, memflags);
>
> May I ask that you omit the unnecessary parentheses?
>
Oh, sorry. I'll remove it, loopy head sometimes...
> Jan
Cheers
Penny
