Re: [PATCH v9 14/41] mm: swap: Introduce lru_add_drain_progressive()

On 7/31/26 15:03, Ackerley Tng wrote:
> "David Hildenbrand (Arm)" <[email protected]> writes:
> 
>> On 7/31/26 12:22, David Hildenbrand (Arm) wrote:
>>>
>>> Just nasty :)
>>>
>>>
>>> mlocked folios in the mlock cache hold a reference as well.
>>>
>>>
>>> I was trying to avoid messing with the refcount for ordinary LRU cache pages.
>>> mlock() should be a corner case for guest_memfd.
>>>
>>> Relying on the refcount just means that one unconditionally performs a lot of LRU
>>> cache draining even though it doesn't make any sense.
>>>
>>>
>>> Again, the problem is that on *any* raised reference you would drain. I
>>> was trying to limit the harm.
>>>
>>> [...]
>>>
>>>
>>> Why?
>>>
>>>
>>> Why?
>>>
>>> That's literally what the existing refcount check tries to do: figure out if
>>> there are LRU caches.
>>>
>>>
>>> I really don't understand what you are trying to say.
>>>
>>> Draining only makes sense if something is on the LRU cache. And there are
>>> better ways of checking that than relying only on even less precise refcounts.
>>>
>>> If someone wants to do an early refcount check to abort the overall
>>> operation, that's fine.
>>>
>>>
>>> ?!
>>>
>>> That's why we have the universal definition of expected references and the
>>> common helper.
>>>
>>> Because pagecache pages commonly don't support the swapcache.
>>>
>>>
>>> It must, because that is used all over the place. The only thing it
>>> cannot deal with is references held by the caller (which could be supplied
>>> through and "additional references" parameter like we do elsewhere).
>>>
>>>
>>> You're not seriously proposing such an abomination I hope?
>>>
>>>
>>> You can just check early for mappings.
>>>
>>> Remember: this is about LRU draining, *not* about your final
>>> "unexpected references" check.
>>>
>>>
>>> Who cares if we end up with a common usable helper? We have usless
>>> checks *all over the place* in common helpers.
>>>
>>>
>>> Yuk.
>>>
>>>
>>> Putting an enum into a header is a problem in which universe? :)
>>>
>>> Ackerley, please stop making up stuff. Having generic helper is not a problem. Doing
>>> checks in common helpers is not a problem. Putting enums in headers is not a problem.
>>>
>>> Your version is just bad.
>>>
>>> I can later try something that keeps the questionable refcount checks in place,
>>> maybe that could do as a temporary solution until Hugh possibly finds a way to
>>> remove the need for draining entirely.
>>>
>>
>> And looking at it, the current folio_ref_count() in GUP is entirely wrong
>> and always makes us drain local+all.
>>
>> Let me fix that first, which will also modify this code.
>>
>> I'd say, to unblock gmem we could have something minimal that I would clean
>> up later, but I think the problem is that lru_add_drain() / lru_add_drain_all()
>> is not exported, right?
>>
> 
> My bad, I totally mixed up refcounting to drain with refcounting for the
> safe conversion and didn't have a complete understanding of mlock().
> 
> The problem for gmem was that if there was an elevated refcount on some
> folio in the range, it might be because the folio was in the lru_add
> fbatch.

Right.

> 
> If it were in the lru_add fbatch, it would be fine to just drain the
> lru_add fbatch and proceed with the conversion.
> 
> lru_add_drain() isn't sufficient since the conversion might be handled
> on one CPU when the folio is on another CPU's fbatch, so I had to do
> lru_add_drain_all(), but lru_add_drain_all() causes IPIs which are
> expensive.

Yes.

You should probably do an early

folio_maybe_dma_pinned() || folio_mapped() check and just return -- don't drain.

> 
> In an earlier revision I exported lru_add_drain_all(). My understanding
> of the discussion at guest_memfd biweekly was that Sean didn't want us
> to export lru_add_drain_all() now, and then end up exporting some other
> function and unexporting lru_add_drain_all(). To avoid exporting and
> unexporting, we then said we should refactor now.
> 
> Happy to go with your proposal too.
Yeah, I have to look into this more closely. I'm now convinced that we really
have to use the refcount for now, but hopefully we can limit it on
folio_expected_refcount() internally + additional references
from the caller.

I'm quite busy today, but let me flesh something out that actually works.

-- 
Cheers,

David