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

[David Hildenbrand <david@redhat.com>]

On 04.10.21 15:45, Matthew Wilcox (Oracle) wrote:
> Make struct slab independent of struct page.  It still uses the
> underlying memory in struct page for storing slab-specific data,
> but slab and slub can now be weaned off using struct page directly.
> Some of the wrapper functions (slab_address() and slab_order())
> still need to cast to struct page, but this is a significant
> disentanglement.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>   include/linux/mm_types.h   | 56 +++++++++++++++++++++++++++++
>   include/linux/page-flags.h | 29 +++++++++++++++
>   mm/slab.h                  | 73 ++++++++++++++++++++++++++++++++++++++
>   mm/slub.c                  |  8 ++---
>   4 files changed, 162 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 7f8ee09c711f..c2ea71aba84e 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -239,6 +239,62 @@ struct page {
>   #endif
>   } _struct_page_alignment;
>   
> +/* Reuses the bits in struct page */
> +struct slab {
> +	unsigned long flags;
> +	union {
> +		struct list_head slab_list;
> +		struct {	/* Partial pages */
> +			struct slab *next;
> +#ifdef CONFIG_64BIT
> +			int slabs;	/* Nr of slabs left */
> +			int pobjects;	/* Approximate count */
> +#else
> +			short int slabs;
> +			short int pobjects;
> +#endif
> +		};
> +		struct rcu_head rcu_head;
> +	};
> +	struct kmem_cache *slab_cache; /* not slob */
> +	/* Double-word boundary */
> +	void *freelist;		/* first free object */
> +	union {
> +		void *s_mem;	/* slab: first object */
> +		unsigned long counters;		/* SLUB */
> +		struct {			/* SLUB */
> +			unsigned inuse:16;
> +			unsigned objects:15;
> +			unsigned frozen:1;
> +		};
> +	};
> +
> +	union {
> +		unsigned int active;		/* SLAB */
> +		int units;			/* SLOB */
> +	};
> +	atomic_t _refcount;
> +#ifdef CONFIG_MEMCG
> +	unsigned long memcg_data;
> +#endif
> +};

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?

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

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.

-- 
Thanks,

David / dhildenb