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

[Matthew Wilcox <willy@infradead.org>]

On Tue, Oct 12, 2021 at 09:25:02AM +0200, David Hildenbrand wrote:
> > > 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.

OK, I think I see what you want:

struct slab {
	struct page_header _1;
	... slab specific stuff ...
	struct page_footer _2;
};

and then cast struct slab to struct page inside slab_nid(),
slab_address(), slab_pgdat(), slab_order() and so on.

To a certain extent, I think that's just an implementation detail; the
important part is the use of struct slab, and then calling slab_nid()
instead of page_nid().  A wrinkle here is that slab does use some of
the bits in page->flags for its own purposes, eg PG_active becomes
PG_pfmemalloc for PageSlab pages.

One of the things I did in the folio patches that I'm not too fond of
now is:

struct folio {
	union {
		struct {
			...
		};
		struct page page;
	};
};

so that I could do &folio->page instead of casting to struct page.
But maybe both of these approaches are just bad ideas, and I should do:

static inline void slab_clear_pfmemalloc(struct slab *slab)
{
	PageClearActive(slab_page(slab));
}

instead of the current:
	clear_bit(PG_pfmemalloc, &slab->flags);