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

[David Hildenbrand <david@redhat.com>]

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