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

[David Hildenbrand <david@redhat.com>]

On 21.08.25 15:20, Matthew Wilcox wrote:
> 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.

Ah, yes, obviously. I misread your mail, assuming you would argue for a 
further change.

-- 
Cheers

David / dhildenb