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

[David Hildenbrand (Arm)]

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.

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

Sure, but it makes the code more consistent and from a GUP PoV easier to understand.

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

That's likely more work, can you look into that?


-- 
Cheers,

David