Re: [RFC PATCH v3 3/8] mm/gup: split follow_page_pte_commit() out of follow_page_pte()

On Wed, 2026-08-12 at 15:23 +0200, David Hildenbrand (Arm) wrote:
> On 8/12/26 15:02, Rik van Riel wrote:
> > On Wed, 2026-08-12 at 13:50 +0200, David Hildenbrand (Arm) wrote:
> > > On 8/11/26 04:51, Rik van Riel wrote:
> > > > 
> > > > +/*
> > > > + * The caller has already run every per-PTE safety check
> > > > (present,
> > > > + * write-fault, gup_must_unshare()) on the PTE, so this only
> > > > does
> > > > the
> > > > + * per-folio work: the refcount grab, the FOLL_PIN
> > > > accessibility
> > > > fault-in,
> > > > + * dirty/accessed marking, and the array fill with the cache
> > > > flush.
> > > > + */
> > > 
> > > If a function needs 4 lines of internal doc to understand what it
> > > does, then
> > > maybe it's not the right function name or abstraction. :)
> > > 
> > > I don't particularly enjoy the "commit" terminology, it looses
> > > the
> > > detail of the
> > > most important thing this function is supposed to to: grab a
> > > folio
> > > reference.
> > > 
> > 
> > Should we rename it to gup_grab_folio() ?
> 
> It does more than grabbing, hm, let me think.
> 
> gup_grab_and_store_folio_pages()
> 
> not sure.

It does more than grabbing, but if we want to
have the PUD path use this same function, we
will probably want to move the gup_fill_pages()
call to outside the lock.

At that point we'll be left with grabbing the
pages, and marking the folio dirty/accessed,
plus the arch_make_folio_accessible() call,
which seems to be a noop on anything but s390,
where it makes the page accessible to the
hypervisor (not sure when/why GUP needs this).
> 
> > 
> > Looking at the differences between the pte and pmd
> > code, there is another big thing that stands out.
> > 
> > The pte code will call mark_folio_dirty() when
> > needed, but the pmd and pud code do not.
> 
> Yes, and my gut feeling is that the same helper should be also used
> in the other
> places. Maybe there is no real bug there, but the inconsistency is
> concerning.

The page table dirty bits should ensure that we
don't lose any data written to the page, but not
marking the folio dirty might mess with dirty
balancing.

-- 
All Rights Reversed.