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

[David Hildenbrand (Arm)]

On 11/3/26 20:30, Rik van Riel wrote:
> On Tue, 2026-11-03 at 18:04 +0100, David Hildenbrand (Arm) wrote:
>> On 11/3/26 18:00, Rik van Riel wrote:
>>>
>>> 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()

That sounds like the right cleanup.

Likely you can defer 1) and just do 2) + 3) first.

1) then on top of the gup-fast batching if possible.

-- 
Cheers,

David