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

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

> 
> 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.

-- 
Cheers,

David