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);
