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