On 12.10.21 16:13, Matthew Wilcox wrote:
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));
}
Yes, that's what I meant.
--
Thanks,
David / dhildenb
