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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 7/31/26 12:22, David Hildenbrand (Arm) wrote:
>>
>> I didn't really like this either, the drain_state thing is really
>> awkward, but in both usages (collect_longterm_unpinnable_folios() and
>> guest_memfd), there's an outer loop where if the draining happened on
>> the local CPU before it should skip straight to just draining on all the
>> other CPUs.
> 
> Just nasty :)
> 
>>
>>> I was hoping that we could embed more logic in a helper. The history [1] of the
>>> refcount check is rather sad:
>>>
>>> https://lore.kernel.org/all/c5bac539-fd8a-4db7-c21c-cd3e457eee91@google.com/
>>>
>>> ... primarily because of mlock() handling.
>>>
>>
>> In the original code in collect_longterm_unpinnable_folios(), I couldn't
>> find anything that handles folio_test_mlock(), so I was lost for a while
>> until I realized mlock() doesn't add a refcount to the folio.
> 
> mlocked folios in the mlock cache hold a reference as well.
> 
>>
>> Are you kind of proposing an optimization to
>> collect_longterm_unpinnable_folios() by adding a check for
>> !folio_test_mlock()? I hope we can put that in a separate patch series,
>> I'm still hoping to get this series in for 7.3!!
> 
> 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.
> 
>>
>>> For guest_memfd(), would mlock() ever apply on a path where you need that check?
>>>
>>
>> For guest_memfd, if a folio were mlocked, unmapping it would fail, and
>> so the refcount on the folio would be elevated. The
>> kvm_gmem_is_safe_for_conversion() check in the patch after this one
>> would fail, correctly.
>>
>> I really wanted the caller of the lru_add_drain_progressive() function
>> to control whether to do the drain (see below), which would solve the
>> mlock problem by not assuming the use of folio_expected_ref_count() to
>> determine whether to do draining.
> 
> Again, the problem is that on *any* raised reference you would drain. I
> was trying to limit the harm.
> 
> [...]
> 
>>> + */
>>> +static void lru_cache_drain_for_folio(const struct folio *folio,
>>> +		enum lru_cache_drained *drained)
>>
>> The main thing I wanted in the proposed version
>> (lru_add_drain_progressive()), was to let the caller determine whether
>> to try or to continue draining.
> 
> Why?
> 
>>
>> I wanted the caller to have full control over whether to drain or not,
>> so I didn't want to pass folio into the function. I thought the caller
>> should first determine whether to drain, then call the function.
> 
> Why?
> 
> That's literally what the existing refcount check tries to do: figure out if
> there are LRU caches.
> 
>>
>> This version assumes that the caller wants to continue draining based on
>> something to do with a folio, and the folio may or may not be on the
>> lru_add fbatch at all, which is a little strange to me.
> 
> 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.
> 
>>
>> Also, this version enforces a certain definition of "expected" number of
>> refcounts on the folio. I guess in this case this definition works with
>> guest_memfd, but guest_memfd doesn't support swap so the swapcache check
>> in folio_expected_ref_count() isn't necessary.
> 
> ?!
> 
> That's why we have the universal definition of expected references and the
> common helper.
> 
> Because pagecache pages commonly don't support the swapcache.
> 
>> Also, if the definition
>> folio_expected_ref_count() changes, then guest_memfd is implicitly
>> affected. Not sure if the "expected" definition is the same for all
>> callers?
> 
> 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).
> 
>>
>> I guess if there was a function named
>> folio_ref_counts_indicate_presence_only_in_the_filemap() instead of
>> folio_expected_ref_count(), then it'd be the perfect function for
> 
> You're not seriously proposing such an abomination I hope?
> 
>> guest_memfd to use. The current definition of folio_expected_ref_count()
>> also includes page table mappings, but in this check guest_memfd really
>> wants to make sure that there are no page table mappings.
> 
> You can just check early for mappings.
> 
> Remember: this is about LRU draining, *not* about your final
> "unexpected references" check.
> 
>>
>> This version folds folio_likely_lru_cached() into the check, and I
>> adopted the check for guest_memfd because it'd help with huge pages,
>> though technically guest_memfd is always 4K now so the check is also
>> pointless.
> 
> Who cares if we end up with a common usable helper? We have usless
> checks *all over the place* in common helpers.
> 
>>
>> I tried a macro version of this where the macro caller can pass in a
>> full condition, which would look like this:
>>
>>   lru_add_drain_while(folio_may_be_lru_cached(folio) &&
>>                       folio_ref_count(folio) != expected_refcount,
>>                       drain_state);
> 
> Yuk.
> 
>>
>> but I thought that just created something people have to jump to, to
>> first understand how the macro works, so I left it as an explicit while
>> loop instead.
>>
>>> +{
>>> +	if (!folio_likely_lru_cached(folio))
>>> +		return false;
>>> +
>>> +	/* Try local draining first, if not already done previously. */
>>> +	if (*drained == LRU_CACHE_NOT_DRAINED) {
>>> +		lru_add_drain();
>>> +		*drained = LRU_CACHE_DRAINED;
>>> +	}
>>> +	/* Try draining all CPUs next if still not an LRU folio. */
>>> +	if (folio_likely_lru_cached(folio) && *drained == LRU_CACHE_DRAINED) {
>>> +		lru_add_drain_all();
>>> +		*drained = LRU_CACHE_DRAINED_ALL;
>>> +	}
>>> +}
>>> +
>>>  /*
>>>   * Returns the number of collected folios. Return value is always >= 0.
>>>   */
>>> @@ -2266,9 +2316,9 @@ static unsigned long collect_longterm_unpinnable_folios(
>>>  		struct list_head *movable_folio_list,
>>>  		struct pages_or_folios *pofs)
>>>  {
>>> +	enum lru_cache_drained drained = LRU_CACHE_NOT_DRAINED;
>>
>> I also considered an enum, but it would be another thing to export. I
>> was thinking to have drain_state just be opaque to the caller and the
>> only thing the caller needs to know is to initialize it to 0.
>>
>> Perhaps there's a better way to "make it opaque"?
> 
> 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?

-- 
Cheers,

David