On 12.11.24 14:53, Jason Gunthorpe wrote:
On Tue, Nov 12, 2024 at 10:10:06AM +0100, David Hildenbrand wrote:
On 12.11.24 06:26, Matthew Wilcox wrote:
On Mon, Nov 11, 2024 at 08:26:54AM +0000, Fuad Tabba wrote:
Thanks for your comments Jason, and for clarifying my cover letter
David. I think David has covered everything, and I'll make sure to
clarify this in the cover letter when I respin.
I don't want you to respin. I think this is a bad idea.
I'm hoping you'll find some more time to explain what exactly you don't
like, because this series only refactors what we already have.
I enjoy seeing the special casing (especially hugetlb) gone from mm/swap.c.
I don't particularly enjoy overlaying folio->lru, primarily because we have
to temporarily "evacuate" it when someone wants to make use of folio->lru
(e.g., hugetlb isolation). So it's not completely "sticky", at least for
hugetlb.
This is really the worst part of it though
Yes.
And, IMHO, seems like overkill. We have only a handful of cases -
maybe we shouldn't be trying to get to full generality but just handle
a couple of cases directly? I don't really think it is such a bad
thing to have an if ladder on the free path if we have only a couple
things. Certainly it looks good instead of doing overlaying tricks.
I'd really like to abstract hugetlb handling if possible. The way it
stands it's just very odd.
We'll need some reliable way to identify these folios that need care.
guest_memfd will be using folio->mapcount for now, so for now we
couldn't set a page type like hugetlb does.
Also how does this translate to Matthew's memdesc world?
guest_memfd and hugetlb would be operating on folios (at least for now),
which contain the refcount,lru,private, ... so nothing special there.
Once we actually decoupled "struct folio" from "struct page", we *might*
have to play less tricks, because we could just have a callback pointer
there. But well, maybe we also want to save some space in there.
Do we want dedicated memdescs for hugetlb/guest_memfd that extend folios
in the future? I don't know, maybe.
I'm currently wondering if we can use folio->private for the time being.
Either
(a) If folio->private is still set once the refcount drops to 0, it
indicates that there is a freeing callback/owner_ops. We'll have to make
hugetlb not use folio->private and convert others to clear
folio->private before freeing.
(b) Use bitX of folio->private to indicate that this has "owner_ops"
meaning. We'll have to make hugetlb not use folio->private and make
others not use bitX. Might be harder and overkill, because right now we
only really need the callback when refcount==0.
(c) Use some other indication that folio->private contains folio_ops.
--
Cheers,
David / dhildenb
