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

[David Hildenbrand (Arm)]

On 11/3/26 17:29, Rik van Riel wrote:
> 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@xxxxxxxxxx>
>>> Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
>>> Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
>>> ---
>>>  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.

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.

> 
>>
>>> +
>>> +	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.

Right. What a mess. Once again :)

> 
> 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...

It's all very inconsistent. And undocumented. And error prone.

Could we have GUP-fast also flush the dcache for consistency or
must that only be called with the PT lock held?


> 
>>
>> 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@xxxxxxx/
> 
> Should both paths be using the same gup_fill_pages()
> helper?

We also have gup_fast_pmd_leaf that fills an array. I think all of these should
just use a helper (instead of manually looping).

For now we can distinguish gup vs. gup-fast using the VMA argument.

-- 
Cheers,

David