Re: [PATCH 1/8] mm/gup: break out gup_fill_pages() helper

[Rik van Riel <riel@surriel.com>]

On Tue, 2026-11-03 at 09:43 +0100, David Hildenbrand (Arm) wrote:
> On 11/2/26 20:30, Rik van Riel wrote:
> > __get_user_pages() fills pages[] and flushes each page's caches in
> > an
> > open-coded loop.
> > 
> > Move it into a gup_fill_pages() helper, which the
> > follow_page_mask()
> > call chain can then use to fill its own pages[] slots.
> > 
> > No functional changes intended.
> > 
> > Suggested-by: David Hildenbrand <david@kernel.org>
> > Reviewed-by: Suren Baghdasaryan <surenb@google.com>
> > Signed-off-by: Rik van Riel <riel@surriel.com>
> > ---
> >  mm/gup.c | 27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 7d43ce717b246..cdbe2ed3e6442 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -633,6 +633,23 @@ static struct page *no_page_table(struct
> > vm_area_struct *vma,
> >  	return NULL;
> >  }
> >  
> > +static void gup_fill_pages(struct vm_area_struct *vma, unsigned
> > long address,
> > +		struct page *page, unsigned long nr, struct page
> > **pages)
> 
> unsigned long nr_pages ?

Will do. Thank you.
> 
> 
> > +{
> > +	unsigned long i;
> > +
> > +	if (!pages)
> > +		return;
> 
> Why is that check needed? Why should someone try filling something
> that doesn't
> exist?

faultin_page_range() and populate_vma_page_range
both call the GUP code with a NULL pages pointer.

> 
> > +
> > +	for (i = 0; i < nr; i++) {
> > +		struct page *subpage = page + i;
> 
> There is no such thing as a subpage as Willy keeps saying :)
> 
> Why not reuse "page" ?
> 
> 	for (i = 0; i < nr; i++, page++) {

Will do.

> 
> > +
> > +		pages[i] = subpage;
> > +		flush_anon_page(vma, subpage, address + i *
> > PAGE_SIZE);
> > +		flush_dcache_page(subpage);
> 
> Now I wonder why GUP-fast doesn't need any of that. Likely because
> architectures
> that support GUP-fast don't need any such magic.

These cache flushes seem to be common on architectures
with virtually indexed caches, where the cache needs 
to be flushed if the memory is accessed through a 
different virtual address than where userspace might
have written data.

On 32 bit ARM, MIPS, and sh, the kernel seems to
implement an actual cache flush here, but also
claims to support GUP-fast.

I don't understand how that would possibly be safe,
since the pages returned by GUP-fast could be
accessed under a different cache alias than where
userspace accesses the memory.

I think one reason GUP-fast can get away without
a dcache flush is that most of the callers take
care of that, for example by using raw_copy_to_user()
to copy data using the user's virtual address, which
takes care of VIVT cache aliasing.

Likewise, drivers that do zero-copy DMA into user
memory will get the cache flushed on VIVT architectures
inside dma_unmap().

The dcache flush inside GUP covers rarer cases, where
the kernel accesses memory at a different address than
where userspace wrote to that memory.

 cat /proc/<pid>/cmdline              KERNEL
 ----                                 -------------------
 read(buf=U)
   -------------------------------->
                                      get_mm_cmdline(mm, U):
                                        access_remote_vm(mm, arg_start,
                                          kbuf, len, FOLL_ANON):
                                          get_user_page_vma_remote()
                                          (slow GUP + its flushes)
                                          kmap_local_folio(P)
                                          memcpy(kbuf <- K)   // read
                                          kunmap_local(K)
                                        copy_to_user(U, kbuf) // write
                                          through U itself
 read buf at U
   ------>
            clean: the final write to U went
            through U. Same line. No hazard.

In this case, the dcache flush in slow GUP ensures
that data written before the GUP is visible to the
reader.

In the write path, e.g. for PTRACE_POKE, __access_remote_vm
flushes after the write has been done, so the other
process will see the data on the next read.

It looks like process_vm_writev() might be missing that
cache flush after a write, though.

However, now we have a situation where:
1) Only a few oddball callers of GUP need the dcache flush, and
2) Those need to do their OWN flush after a write, and
3) The dcache flush is not needed for most other GUP callers.

We could do two different things here to solve the
asymmetry.

Either we could add dcache flushing to 
unpin_user_pages_dirty_lock(), and make sure all
the callers that need to call that function, or
we could move the cache flushing to the GUP callers.

Then we have KSM, which uses GUP, and which would like
to see the latest page contents...

> 
> Maybe we should in fact base this on the gup-fast batching series and
> also use
> the helper in there (passing vma=NULL)?

I just moved this code out of __get_user_pages(),
but if we can get rid of it completely by using
a different helper, all the better.

Which GUP-fast batching series are you talking
about, here?

In Yeoreum Yun's series, the batch filling seems 
to be done inline in gup_fast_pte_range(), and not
in a helper:

https://lore.kernel.org/all/20261102-refactor_gup_fast-v2-4-bc1ac018c834@arm.com/

Should both paths be using the same gup_fill_pages()
helper?

The cache flushing part is broken out into its own
helper function later in this series, so we could
sequence those differently.

-- 
All Rights Reversed.