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

[Rik van Riel]

On Tue, 2026-11-03 at 17:39 +0100, David Hildenbrand (Arm) wrote:
> 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:
> > > 
> > > 
> > > 
> > > > +{
> > > > +	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.

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?

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

I think we could, but it does not look like any of
them need it.

Out of the 83 callers of gup_fast, many DMA into the
page, and the drivers do their own flushing. Also,
flushing before the DMA write doesn't help.

KVM, Infiniband, and others pin the pages, sometimes
for longer periods of time, and either flush around 
their reads and writes, or run only on architectures
where we don't need to.

The others access the page through the user's own
virtual address, which again does not require a
dcache flush.

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.

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

I think all of those could use the same helper
function I am breaking out here, once the dcache
flushing moves to its own helper.

-- 
All Rights Reversed.