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/[email protected]/ >>> >>> ... 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
