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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -131,12 +131,18 @@ int query_page_offline(mfn_t mfn, uint32_t *status);
>  
>  void heap_init_late(void);
>  
> -int assign_pages(
> +int assign_pages_nr(
>      struct domain *d,
>      struct page_info *pg,
> -    unsigned int order,
> +    unsigned int nr_pfns,
>      unsigned int memflags);
>  
> + int assign_pages(
> +     struct domain *d,
> +     struct page_info *pg,
> +     unsigned int order,
> +     unsigned int memflags);

Bogus extra leading space on all lines of this addition.

Jan