Re: [PATCHv2 6/7] mm/gup: Combine parameters into struct

[Dan Williams]

On Thu, Sep 20, 2018 at 1:03 PM Keith Busch <keith.busch@xxxxxxxxx> wrote:
>
> This will make it easier to add new parameters that we may wish to
> thread through these function calls.
>
> Cc: Kirill Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Signed-off-by: Keith Busch <keith.busch@xxxxxxxxx>
> ---
>  include/linux/huge_mm.h |  12 +--
>  include/linux/hugetlb.h |   2 +-
>  include/linux/mm.h      |  21 ++++-
>  mm/gup.c                | 238 +++++++++++++++++++++++-------------------------
>  mm/huge_memory.c        |  32 +++----
>  mm/nommu.c              |   6 +-
>  6 files changed, 151 insertions(+), 160 deletions(-)
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index 99c19b06d9a4..7d22e2c7f154 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -212,10 +212,8 @@ static inline int hpage_nr_pages(struct page *page)
>         return 1;
>  }
>
> -struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long addr,
> -               pmd_t *pmd, int flags);
> -struct page *follow_devmap_pud(struct vm_area_struct *vma, unsigned long addr,
> -               pud_t *pud, int flags);
> +struct page *follow_devmap_pmd(struct follow_page_context *ctx, pmd_t *pmd);
> +struct page *follow_devmap_pud(struct follow_page_context *ctx, pud_t *pud);
>
>  extern vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf, pmd_t orig_pmd);
>
> @@ -343,14 +341,12 @@ static inline void mm_put_huge_zero_page(struct mm_struct *mm)
>         return;
>  }
>
> -static inline struct page *follow_devmap_pmd(struct vm_area_struct *vma,
> -               unsigned long addr, pmd_t *pmd, int flags)
> +static inline struct page *follow_devmap_pmd(struct gup_context *ctx, pmd_t *pmd)
>  {
>         return NULL;
>  }
>
> -static inline struct page *follow_devmap_pud(struct vm_area_struct *vma,
> -               unsigned long addr, pud_t *pud, int flags)
> +static inline struct page *follow_devmap_pud(struct gup_context *ctx, pud_t *pud)
>  {
>         return NULL;
>  }
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 6b68e345f0ca..64b675863793 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -180,7 +180,7 @@ static inline void hugetlb_report_meminfo(struct seq_file *m)
>  static inline void hugetlb_show_meminfo(void)
>  {
>  }
> -#define follow_huge_pd(vma, addr, hpd, flags, pdshift) NULL
> +#define follow_huge_pd(ctx, hpd, pdshift)      NULL
>  #define follow_huge_pmd(mm, addr, pmd, flags)  NULL
>  #define follow_huge_pud(mm, addr, pud, flags)  NULL
>  #define follow_huge_pgd(mm, addr, pgd, flags)  NULL
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a61ebe8ad4ca..f1fd241c9071 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -378,6 +378,13 @@ struct vm_fault {
>                                          */
>  };
>
> +struct follow_page_context {
> +       struct vm_area_struct *vma;
> +       unsigned long address;
> +       unsigned int page_mask;
> +       unsigned int flags;
> +};

I was thinking that the context would only contain the in/out
parameters of page_mask and pgmap. I don't think we plan to have more
parameters beyond this one addition and from a code readability
standpoint the less "ctx->" the better.