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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

> --- 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)
{

?

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

Jan