Message-ID: <CAFgQCTtVcmLUdua_nFwif_TbzeX5wp31GfTpL6CWmXXviYYLyw@mail.gmail.com>
Date:   Fri, 31 May 2019 19:05:27 +0800
From:   Pingfan Liu <kernelfans@...il.com>
To:     John Hubbard <jhubbard@...dia.com>
Cc:     Ira Weiny <ira.weiny@...el.com>, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Matthew Wilcox <willy@...radead.org>,
        "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>,
        Keith Busch <keith.busch@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm/gup: fix omission of check on FOLL_LONGTERM in get_user_pages_fast()

On Fri, May 31, 2019 at 7:21 AM John Hubbard <jhubbard@...dia.com> wrote:
>
> On 5/30/19 2:47 PM, Ira Weiny wrote:
> > On Thu, May 30, 2019 at 06:54:04AM +0800, Pingfan Liu wrote:
> [...]
> >> +                            for (j = i; j < nr; j++)
> >> +                                    put_page(pages[j]);
> >
> > Should be put_user_page() now.  For now that just calls put_page() but it is
> > slated to change soon.
> >
> > I also wonder if this would be more efficient as a check as we are walking the
> > page tables and bail early.
> >
> > Perhaps the code complexity is not worth it?
>
> Good point, it might be worth it. Because now we've got two loops that
> we run, after the interrupts-off page walk, and it's starting to look like
> a potential performance concern.
>
> >
> >> +                            nr = i;
> >
> > Why not just break from the loop here?
> >
> > Or better yet just use 'i' in the inner loop...
> >
>
> ...but if you do end up putting in the after-the-fact check, then we can
> go one or two steps further in cleaning it up, by:
>
>     * hiding the visible #ifdef that was slicing up gup_fast,
>
>     * using put_user_pages() instead of either put_page or put_user_page,
>       thus getting rid of j entirely, and
>
>     * renaming an ancient minor confusion: nr --> nr_pinned),
>
> we could have this, which is looks cleaner and still does the same thing:
>
> diff --git a/mm/gup.c b/mm/gup.c
> index f173fcbaf1b2..0c1f36be1863 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1486,6 +1486,33 @@ static __always_inline long __gup_longterm_locked(struct task_struct *tsk,
>  }
>  #endif /* CONFIG_FS_DAX || CONFIG_CMA */
>
> +#ifdef CONFIG_CMA
> +/*
> + * Returns the number of pages that were *not* rejected. This makes it
> + * exactly compatible with its callers.
> + */
> +static int reject_cma_pages(int nr_pinned, unsigned gup_flags,
> +                           struct page **pages)
> +{
> +       int i = 0;
> +       if (unlikely(gup_flags & FOLL_LONGTERM)) {
> +
> +               for (i = 0; i < nr_pinned; i++)
> +                       if (is_migrate_cma_page(pages[i])) {
> +                               put_user_pages(&pages[i], nr_pinned - i);
> +                               break;
> +                       }
> +       }
> +       return i;
> +}
> +#else
> +static int reject_cma_pages(int nr_pinned, unsigned gup_flags,
> +                           struct page **pages)
> +{
> +       return nr_pinned;
> +}
> +#endif
> +
>  /*
>   * This is the same as get_user_pages_remote(), just with a
>   * less-flexible calling convention where we assume that the task
> @@ -2216,7 +2243,7 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
>                         unsigned int gup_flags, struct page **pages)
>  {
>         unsigned long addr, len, end;
> -       int nr = 0, ret = 0;
> +       int nr_pinned = 0, ret = 0;
>
>         start &= PAGE_MASK;
>         addr = start;
> @@ -2231,25 +2258,27 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
>
>         if (gup_fast_permitted(start, nr_pages)) {
>                 local_irq_disable();
> -               gup_pgd_range(addr, end, gup_flags, pages, &nr);
> +               gup_pgd_range(addr, end, gup_flags, pages, &nr_pinned);
>                 local_irq_enable();
> -               ret = nr;
> +               ret = nr_pinned;
>         }
>
> -       if (nr < nr_pages) {
> +       nr_pinned = reject_cma_pages(nr_pinned, gup_flags, pages);
> +
> +       if (nr_pinned < nr_pages) {
>                 /* Try to get the remaining pages with get_user_pages */
> -               start += nr << PAGE_SHIFT;
> -               pages += nr;
> +               start += nr_pinned << PAGE_SHIFT;
> +               pages += nr_pinned;
>
> -               ret = __gup_longterm_unlocked(start, nr_pages - nr,
> +               ret = __gup_longterm_unlocked(start, nr_pages - nr_pinned,
>                                               gup_flags, pages);
>
>                 /* Have to be a bit careful with return values */
> -               if (nr > 0) {
> +               if (nr_pinned > 0) {
>                         if (ret < 0)
> -                               ret = nr;
> +                               ret = nr_pinned;
>                         else
> -                               ret += nr;
> +                               ret += nr_pinned;
>                 }
>         }
>
>
> Rather lightly tested...I've compile-tested with CONFIG_CMA and !CONFIG_CMA,
> and boot tested with CONFIG_CMA, but could use a second set of eyes on whether
> I've added any off-by-one errors, or worse. :)
>
Do you mind I send V2 based on your above patch? Anyway, it is a simple bug fix.

Thanks,
  Pingfan

Powered by blists - more mailing lists