Thread (77 messages) 77 messages, 3 authors, 2021-10-14

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

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help