Re: [PATCH] filemap: optimize order0 folio in filemap_map_pages

[Matthew Wilcox <willy@infradead.org>]

On Thu, Aug 21, 2025 at 02:57:54PM +0200, David Hildenbrand wrote:
> On 21.08.25 14:45, Matthew Wilcox wrote:
> > On Thu, Aug 21, 2025 at 09:22:48AM +0800, Jinjiang Tu wrote:
> > > 在 2025/8/20 20:42, Matthew Wilcox 写道:
> > > > On Wed, Aug 20, 2025 at 09:10:56AM +0800, Jinjiang Tu wrote:
> > > > > We should call folio_unlock() before folio_put(). In filemap_map_order0_folio(),
> > > > > if we doesn't set folio into pte, we should unlock and put folio.
> > > > I agree that folio_unlock() needs to be called before folio_put().
> > > > What I don't understand is why we need to delay folio_unlock() until
> > > > right before folio_put().  Can't we leave folio_unlock() where it
> > > > currently is and only move the folio_put()?
> > > 
> > > In filemap_map_order0_folio(), assuming the page is hwpoisoned, we skip set_pte_range(),
> > > the folio should be unlocked and put. If we only move folio_put() to filemap_map_order0_folio(),
> > > the folio is unlocked when filemap_map_pages() doesn't hold any folio refcount.
> > Oh, I see.  I misread your patch; sorry about that.
> > 
> > However, it is still safe to move only the folio_put() and not move
> > the folio_unlock()!  It's a little subtle, so I'll explain.
> > 
> > We must not free a locked folio.  The page allocator has checks for this
> > and will complain (assuming appropriate debug options are enabled).  So
> > this sequence:
> > 
> > 	folio_put(folio);
> > 	folio_unlock(folio);
> > 
> > is _generally_ unsafe because the folio_put() might be the last put of
> > the refcount which will cause the folio to be freed.  However, if we know
> > that the folio has a refcount > 1, it's safe because the folio_put()
> > won't free the folio.  We do know that the folio has a refcount >1
> > because it's in the page cache, which keeps a refcount on the folio.
> > Since we have it locked, we know that truncation will wait for the unlock
> > to happen, and truncation will be the last one to put the refcount.
> 
> I agree that it is save, but is it worth it having that subtle detail here
> instead of just doing unlock+put?
> 
> IOW, what do we gain by doing it differently? :)

That was in the initial mail:

> With this patch, we can get 8% performance gain for lmbench testcase
> 'lat_pagefault -P 1 file', the size of file is 512M.

Obviously the exact gains are going to depend on how good your CPU is
at doing atomic inc/decs, but reducing the number of atomics is always
a good thing.