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
