Re: [PATCH v6 5/7] xen: re-define assign_pages and introduce a new function assign_page

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.09.2021 11:52, 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
> introduces a new helper assign_page for original page with a single order.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>

I have to admit that I'm now very puzzled: Instead of restoring the
long agreed upon ordering of parameters (and then keeping my A-b),
you've dropped the ack.

> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -568,7 +568,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_pages(d, mfn_to_page(_mfn(mfn++)), 1, 0) )

This change alone demonstrates the problem when it comes to backporting
future changes: If the original patch contained a code addition similar
to what you change to, without the person doing the backporting paying
close attention, the result will be an order-1 request when an order-0
one is wanted. It was explained to you that in order to make people
doing backports aware of this semantic change, the order of parameters
of the function ought to be altered. That way the compiler will
complain, and the person will know to look closely what adjustments are
needed.

In this context I find it further puzzling ...

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -133,8 +133,14 @@ void heap_init_late(void);
>  
>  int assign_pages(
>      struct domain *d,
> +    struct page_info *pg,
> +    unsigned long nr,
> +    unsigned int memflags);
> +
> +int assign_page(
>      struct page_info *pg,
>      unsigned int order,
> +    struct domain *d,
>      unsigned int memflags);

... that you also neglected the request to harmonize the argument order
of both functions. What we want (and what I thought has long been agreed
upon) is e.g.

int assign_pages(
    struct page_info *pg,
    unsigned long nr,
    struct domain *d,
    unsigned int memflags);

int assign_page(
    struct page_info *pg,
    unsigned int order,
    struct domain *d,
    unsigned int memflags);

Jan