Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180920200358.31680-1-keith.busch@intel.com> <20180920200358.31680-7-keith.busch@intel.com>
In-Reply-To: <20180920200358.31680-7-keith.busch@intel.com>
From:   Dan Williams <dan.j.williams@intel.com>
Date:   Fri, 21 Sep 2018 12:45:23 -0700
Message-ID: <CAPcyv4iq+9xCfUi4PMtrjPiBOB5fbtiycp3jX5mkchEgcLNgFg@mail.gmail.com>
Subject: Re: [PATCHv2 6/7] mm/gup: Combine parameters into struct
To:     Keith Busch <keith.busch@intel.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Dave Hansen <dave.hansen@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Sep 20, 2018 at 1:03 PM Keith Busch <keith.busch@intel.com> 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@linux.intel.com>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  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.