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

[Rik van Riel]

On Tue, 2026-11-03 at 18:04 +0100, David Hildenbrand (Arm) wrote:
> On 11/3/26 18:00, Rik van Riel wrote:
> > On Tue, 2026-11-03 at 17:39 +0100, David Hildenbrand (Arm) wrote:
> > > On 11/3/26 17:29, Rik van Riel wrote:
> > > > 
> > > > faultin_page_range() and populate_vma_page_range
> > > > both call the GUP code with a NULL pages pointer.
> > > 
> > > That's not what I meant. I know that we have GUP users without
> > > pages array.
> > > 
> > > In the code change you are performing the check is not required.
> > > 
> > > That's why there wasn't one in the first place.
> > 
> > It looks like 2 of the 3 callers of gup_fill_pages()
> > check for pages existing. The other does not.
> > 
> > Should we just have the check in this one place, and
> > not depend on the caller?
> > 
> 
> That should be spelled out in the patch description.
> 
> The change as it stands in this patch looks useless :)

> Although trying to fill something that doesn't exist is a bit weird.
> 
> But if it makes the calling code nicer, sure.
> 
I agree this patch by itself isn't super useful.

It's goal is to be able to have this code in one
spot, and not in multiple places in gup.c

I'll split out the array filling from the cache
flushing, so we can move the cache flushing out
from under the lock, and have GUP-fast and regular
GUP share the same helper.

> > 
> > 
> > Only a small number of slow GUP callers need dcache
> > flushes.
> > 
> > It may make more sense to move the cache flushes
> > all the way out into __get_user_pages() and away
> > from the follow_page_mask() call chain, so we can
> > maybe merge more of the helper functions between
> > slow GUP and fast GUP.
> 
> That's likely more work, can you look into that?
> 
It really doesn't look too bad. Here are all
the GUP callers that need a cache flush:
- kernel/events/uprobes.c (through get_user_pages_remote)
- /proc/pid/cmdline (through access_remote_vm)
- fs/proc/base.c (through access_remote_vm)
- kernel/trace/trace_events_user.c (pin_user_pages_remote)
- arch/x86/kernel/cpu/sgx/ioctl.c (get_user_pages ... urgh)
- kernel/ptrace.c (access_remote_vm)
- mm/process_vm_access.c (pin_user_pages_remote)
- fs/exec.c (pin_user_pages_remote)
- virt/kvm/async_pf.c (get_user_pages_remote)

In other words, if we change the SGX code to just
call get_user_pages_remote(), and then move the
cache flushing to get_user_pages_remote() and
pin_user_pages_remote() we should be all set.

Want me to fold that into this series, or send
it in as a small, separate prequel series?

As a separate series it would:
1) split out the gup_fill_pages() and gup_flush_pages()
   helpers
2) convert the SGX code to use get_user_pages_remote()
3) move the flush call up the stack into pin_user_pages_remote()
   and get_user_pages_remote()

Just let me know if you prefer that separate, or
folded in. 

-- 
All Rights Reversed.