Re: [PATCH 03/62] mm: Split slab into its own type

[Matthew Wilcox <willy@infradead.org>]

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.