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.