Re: [PATCH 03/62] mm: Split slab into its own type
From: David Hildenbrand <hidden>
Date: 2021-10-05 16:10:34
On 04.10.21 15:45, Matthew Wilcox (Oracle) wrote:
quoted hunk ↗ jump to hunk
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