Re: [PATCH v16 3/9] mm/gup: Introduce memfd_pin_folios() for pinning memfd folios

[SeongJae Park <sj@kernel.org>]

Hello Vivek and Andrew,

On Sun, 23 Jun 2024 23:36:11 -0700 Vivek Kasireddy <vivek.kasireddy@intel.com> wrote:

> For drivers that would like to longterm-pin the folios associated
> with a memfd, the memfd_pin_folios() API provides an option to
> not only pin the folios via FOLL_PIN but also to check and migrate
> them if they reside in movable zone or CMA block. This API
> currently works with memfds but it should work with any files
> that belong to either shmemfs or hugetlbfs. Files belonging to
> other filesystems are rejected for now.
> 
> The folios need to be located first before pinning them via FOLL_PIN.
> If they are found in the page cache, they can be immediately pinned.
> Otherwise, they need to be allocated using the filesystem specific
> APIs and then pinned.
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> Cc: Junxiao Chang <junxiao.chang@intel.com>
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> (v2)
> Reviewed-by: David Hildenbrand <david@redhat.com> (v3)
> Reviewed-by: Christoph Hellwig <hch@lst.de> (v6)
> Acked-by: Dave Airlie <airlied@redhat.com>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  include/linux/memfd.h |   5 ++
>  include/linux/mm.h    |   3 +
>  mm/gup.c              | 137 ++++++++++++++++++++++++++++++++++++++++++
>  mm/memfd.c            |  45 ++++++++++++++
>  4 files changed, 190 insertions(+)
> 
[...]
> diff --git a/mm/gup.c b/mm/gup.c
> index a88e19c78730..94160abbf499 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
[...]
> @@ -3747,3 +3749,138 @@ long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
>  				     &locked, gup_flags);
>  }
>  EXPORT_SYMBOL(pin_user_pages_unlocked);
> +
> +/**
> + * memfd_pin_folios() - pin folios associated with a memfd
[...]
> +			for (i = 0; i < nr_found; i++) {
> +				/*
> +				 * As there can be multiple entries for a
> +				 * given folio in the batch returned by
> +				 * filemap_get_folios_contig(), the below
> +				 * check is to ensure that we pin and return a
> +				 * unique set of folios between start and end.
> +				 */
> +				if (next_idx &&
> +				    next_idx != folio_index(fbatch.folios[i]))
> +					continue;
> +
> +				folio = try_grab_folio(&fbatch.folios[i]->page,
> +						       1, FOLL_PIN);
> +				if (!folio) {
> +					folio_batch_release(&fbatch);
> +					ret = -EINVAL;
> +					goto err;
> +				}

I found this patch is applied on mm-unstable as commit 7618d1ff59ef ("mm/gup:
introduce memfd_pin_folios() for pinning memfd folios").  Somehow, however, the
commit has changd the above try_grab_folio() call to try_grab_folio_fast()
call.

As a result, building kernel without CONFIG_MMU fais as below:

      CC      mm/gup.o
    mm/gup.c: In function 'memfd_pin_folios':
    mm/gup.c:3862:41: error: implicit declaration of function 'try_grab_folio_fast'; did you mean 'try_grab_folio'? [-Werror=implicit-function-declaration]
     3862 |                                 folio = try_grab_folio_fast(&fbatch.folios[i]->page,
          |                                         ^~~~~~~~~~~~~~~~~~~
          |                                         try_grab_folio
    mm/gup.c:3862:39: warning: assignment to 'struct folio *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     3862 |                                 folio = try_grab_folio_fast(&fbatch.folios[i]->page,
          |                                       ^

But simply changing the call back to try_grab_folio() causes another failure:

      CC      mm/gup.o
    mm/gup.c: In function 'memfd_pin_folios':
    mm/gup.c:3862:56: error: passing argument 1 of 'try_grab_folio' from incompatible pointer type [-Werror=incompatible-pointer-types]
     3862 |                                 folio = try_grab_folio(&fbatch.folios[i]->page,
          |                                                        ^~~~~~~~~~~~~~~~~~~~~~~
          |                                                        |
          |                                                        struct page *
    mm/gup.c:141:47: note: expected 'struct folio *' but argument is of type 'struct page *'
      141 | int __must_check try_grab_folio(struct folio *folio, int refs,
          |                                 ~~~~~~~~~~~~~~^~~~~
    mm/gup.c:3862:39: warning: assignment to 'struct folio *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     3862 |                                 folio = try_grab_folio(&fbatch.folios[i]->page,
          |                                       ^

Maybe the change has made to fix conflict with another mm-unstable commit
02a2d55767d1 ("mm: gup: stop abusing try_grab_folio"), but forgot the
CONFIG_MMU unset case?

I confirmed the failure disappears after further cleanup like below:

diff --git a/mm/gup.c b/mm/gup.c
index 46a266ed84f7..9f4902425070 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -3859,9 +3859,9 @@ long memfd_pin_folios(struct file *memfd, loff_t start, loff_t end,
                                    next_idx != folio_index(fbatch.folios[i]))
                                        continue;

-                               folio = try_grab_folio_fast(&fbatch.folios[i]->page,
-                                                      1, FOLL_PIN);
-                               if (!folio) {
+                               folio = page_folio(&fbatch.folios[i]->page);
+
+                               if (try_grab_folio(folio, 1, FOLL_PIN)) {
                                        folio_batch_release(&fbatch);
                                        ret = -EINVAL;
                                        goto err;

I didn't look deep into the patch, so unsure if that's a valid fix, though.
May I ask your thoughts?


Thanks,
SJ

[...]