On 05.10.21 20:48, Matthew Wilcox wrote:
On Tue, Oct 05, 2021 at 06:10:24PM +0200, David Hildenbrand wrote:
My 2 cents just from reading the first 3 mails:
I'm not particularly happy about the "/* Reuses the bits in struct page */"
part of thingy here, essentially really having to pay attention what
whenever we change something in "struct page" to not mess up all the other
special types we have. And I wasn't particularly happy scanning patch #1 and
#2 for the same reason. Can't we avoid that?
I've tried to mitigate that with the compile-time assertions. They're
actually a bit stronger than what we have now.
Sorry for the late reply.
Okay, that's at least something :)
What I can see is that we want (and must right now for generic
infrastructure) keep some members of the the struct page" (e.g., flags,
_refcount) at the very same place, because generic infrastructure relies on
them.
Maybe that has already been discussed somewhere deep down in folio mail
threads, but I would have expected that we keep struct-page generic inside
struct-page and only have inside "struct slab" what's special for "struct
slab".
I would have thought that we want something like this (but absolutely not
this):
struct page_header {
unsigned long flags;
}
struct page_footer {
atomic_t _refcount;
#ifdef CONFIG_MEMCG
unsigned long memcg_data;
#endif
}
struct page {
struct page_header header;
uint8_t reserved[$DO_THE_MATH]
struct page_footer footer;
};
The problem with this definition is the number of places which refer
to page->flags and must now be churned to page->header.flags.
_refcount is rather better encapsulated, and I'm not entirely sure
how much we'd have to do for memcg_data. Maybe that was what you meant
by "this but absolutely not this"? I don't quite understand what that
was supposed to mean.
Exactly what you mentioned above (changing all callers) is what I didn't
want :)
I was thinking of a way to have these "fixed fields" be defined only
once, and ideally, perform any access to these fields via the "struct
page" instead of via the "struct slab".
Like, when wanting to set a page flag with a slab, access them
((struct page *)slab)->flags
instead of using slab->flags. Essentially not duplicating these fields
and accessing them via the "refined" page types, but only putting a
"reserved" placeholder in. That would mean that we wouldn't need patch
#1 and #2, because we wouldn't be passing around pgflags (using whatever
fancy type we will decide on), all such accesses would continue going
via the "struct page" -- because that's where these common fields
actually reside in right now at places we cannot simply change --
because common infrastructure (PFN walkers, ...) heavily relyies on the
flags, the refcount and even the mapcount (page types) being set
accordingly.
struct slab {
...
};
struct slab_page {
struct page_header header;
struct slab;
struct page_footer footer;
};
Instead of providing helpers for struct slab_page, simply cast to struct
page and replace the structs in struct slab_page by simple placeholders with
the same size.
That would to me look like a nice cleanup itself, ignoring all the other
parallel discussions that are going on. But I imagine the problem is more
involved, and a simple header/footer might not be sufficient.
Yes, exactly, the problems are more involved. The location/contents of
page->mapping are special, the contents of bit zero of the second word
are special (determines compound_head or not) and slub particularly
relies on the 128-bit alignment of the { freelist, counters } pair.
The ultimate destination (and I think Kent/Johannes/I all agree
on this) is to dynamically allocate struct slab. At _that_ point,
we can actually drop _refcount from struct slab and even change how
struct slab is defined based on CONFIG_SLUB / CONFIG_SLOB / CONFIG_SLAB.
I'm also sold on the idea of simplifying "struct page" (even when not
shrinking it!) by moving out these "struct slab" parts that dictate the
"struct page" layout heavily.
I think we'll still need ->flags to be the first element of struct slab,
but bit 0 of the second word stops being special, we won't need to care
about where page->mapping aliases, and the 128-bit alignment becomes
solely the concern of the slub allocator instead of affecting everyone
who uses struct page.
Yes, that sounds good to me.
--
Thanks,
David / dhildenb
