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

[Dan Williams]

On Fri, Sep 21, 2018 at 12:45 PM Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
>
> 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.

Also, the next version of this series should be copied to linux-mm.