RE: [PATCH V3 07/10] xen: re-define assign_pages and introduce assign_page

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, July 19, 2021 4:41 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; julien@xxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> 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