Re: [Xen-devel] [PATCH 2/4] x86/mm: Implement common put_data_pages for put_page_from_l[23]e

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.12.2019 18:32, George Dunlap wrote:
> Both put_page_from_l2e and put_page_from_l3e handle having superpage
> entries by looping over each page and "put"-ing each one individually.
> As with putting page table entries, this code is functionally
> identical, but for some reason different.  Moreover, there is already
> a common function, put_data_page(), to handle automatically swapping
> between put_page() (for read-only pages) or put_page_and_type() (for
> read-write pages).
> 
> Replace this with put_data_pages() (plural), which does the entire
> loop, as well as the put_page / put_page_and_type switch.
> 
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> ---
> NB that I've used the "simple for loop" version to make it easy to see
> what's going on, rather than the "do { } while()" version which uses &
> and compare to zero rather than comparing to the max.
> 
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> ---
>  xen/arch/x86/mm.c | 52 ++++++++++++++++++-----------------------------
>  1 file changed, 20 insertions(+), 32 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index d8a0eb2aa5..c05039ab21 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1289,14 +1289,6 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain 
> *l1e_owner)
>  }
>  
>  #ifdef CONFIG_PV
> -static void put_data_page(struct page_info *page, bool writeable)
> -{
> -    if ( writeable )
> -        put_page_and_type(page);
> -    else
> -        put_page(page);
> -}
> -
>  static int put_pt_page(struct page_info *pg, struct page_info *ptpg,
>                         unsigned int flags)
>  {
> @@ -1319,6 +1311,20 @@ static int put_pt_page(struct page_info *pg, struct 
> page_info *ptpg,
>      return rc;
>  }
>  
> +static int put_data_pages(struct page_info *page, bool writeable, int 
> pt_shift)
> +{
> +    int i, count = 1 << (pt_shift - PAGE_SHIFT);

With both "int" here changed to "unsigned int"
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
But of course Andrew's objection needs addressing one way or another.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel