Re: [PATCH v2 02/15] slab: add struct kmem_cache_args

[Christian Brauner <brauner@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2282 bytes --]

On Wed, Sep 04, 2024 at 11:53:05AM GMT, Linus Torvalds wrote:
> On Wed, 4 Sept 2024 at 11:21, Christian Brauner <brauner@kernel.org> wrote:
> >
> > Sure. So can you fold your suggestion above and the small diff below
> > into the translation layer patch?
> 
> Please don't.
> 
> This seems horrible. First you have a _Generic() macro that turns NULL
> into the same function that a proper __kmem_cache_create_args() with a
> real argument uses, and then you make that function check for NULL and
> turn it into something else.
> 
> That seems *entirely* pointless.
> 
> I think the right model is to either
> 
>  (a) not allow a NULL pointer at all (ie not have a _Generic() case
> for 'void *') and just error for that behavior

Fine by me and what this did originally by erroring out with a compile
time error.

> 
> OR
> 
>  (b) make a NULL pointer explicitly go to some other function than the
> one that gets a proper pointer
> 
> but not this "do extra work in the function to make it accept the NULL
> we shunted to it".
> 
> IOW, something like this:
> 
>   #define kmem_cache_create(__name, __object_size, __args, ...)           \
>        _Generic((__args),                                              \
>                struct kmem_cache_args *: __kmem_cache_create_args,     \
>                void *: __kmem_cache_default_args,                       \
>               default: __kmem_cache_create)(__name, __object_size,
> __args, __VA_ARGS__)
> 
> and then we have
> 
>  static inline struct kmem_cache *__kmem_cache_default_args(const char *name,
>                                            unsigned int object_size,
>                                            struct kmem_cache_args *args,
>                                            slab_flags_t flags)
>   { WARN_ON_ONCE(args); // It had *better* be NULL, not some random 'void *'
>      return __kmem_cache_create_args(name, size, &kmem_args, flags); }
> 
> which basically just does a "turn NULL into &kmem_args" thing.
> 
> Notice how that does *not* add some odd NULL pointer check to the main
> path (and the WARN_ON_ONCE() check should be compiled away for any
> actual constant NULL argument, which is the only valid reason to have
> that 'void *' anyway).

Also fine by me. See appended updated patch.

[-- Attachment #2: 0001-slab-create-kmem_cache_create-compatibility-layer.patch --]
[-- Type: text/x-diff, Size: 4001 bytes --]

From ede72f93668827497fb8d1c0f7286cfb4bd4f204 Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Tue, 3 Sep 2024 14:49:49 +0200
Subject: [PATCH] slab: create kmem_cache_create() compatibility layer

Use _Generic() to create a compatibility layer that type switches on the
third argument to either call __kmem_cache_create() or
__kmem_cache_create_args(). If NULL is passed for the struct
kmem_cache_args argument use default args making porting for callers
that don't care about additional arguments easy.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 include/linux/slab.h | 29 ++++++++++++++++++++++++++---
 mm/slab_common.c     | 10 +++++-----
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index aced16a08700..d406d00cabbb 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -261,9 +261,10 @@ struct kmem_cache *__kmem_cache_create_args(const char *name,
 					    unsigned int object_size,
 					    struct kmem_cache_args *args,
 					    slab_flags_t flags);
-struct kmem_cache *kmem_cache_create(const char *name, unsigned int size,
-			unsigned int align, slab_flags_t flags,
-			void (*ctor)(void *));
+
+struct kmem_cache *__kmem_cache_create(const char *name, unsigned int size,
+				       unsigned int align, slab_flags_t flags,
+				       void (*ctor)(void *));
 struct kmem_cache *kmem_cache_create_usercopy(const char *name,
 			unsigned int size, unsigned int align,
 			slab_flags_t flags,
@@ -272,6 +273,28 @@ struct kmem_cache *kmem_cache_create_usercopy(const char *name,
 struct kmem_cache *kmem_cache_create_rcu(const char *name, unsigned int size,
 					 unsigned int freeptr_offset,
 					 slab_flags_t flags);
+
+/* If NULL is passed for @args, use this variant with default arguments. */
+static inline struct kmem_cache *
+__kmem_cache_default_args(const char *name, unsigned int size,
+			  const struct kmem_cache_args *args,
+			  slab_flags_t flags)
+{
+	struct kmem_cache_args kmem_default_args = {};
+
+	/* Make sure we don't get passed garbage. */
+	if (WARN_ON_ONCE(args))
+		return NULL;
+
+	return __kmem_cache_create_args(name, size, &kmem_default_args, flags);
+}
+
+#define kmem_cache_create(__name, __object_size, __args, ...)           \
+	_Generic((__args),                                              \
+		struct kmem_cache_args *: __kmem_cache_create_args,	\
+		void *: __kmem_cache_default_args,			\
+		default: __kmem_cache_create)(__name, __object_size, __args, __VA_ARGS__)
+
 void kmem_cache_destroy(struct kmem_cache *s);
 int kmem_cache_shrink(struct kmem_cache *s);
 
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 19ae3dd6e36f..418459927670 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -383,7 +383,7 @@ kmem_cache_create_usercopy(const char *name, unsigned int size,
 EXPORT_SYMBOL(kmem_cache_create_usercopy);
 
 /**
- * kmem_cache_create - Create a cache.
+ * __kmem_cache_create - Create a cache.
  * @name: A string which is used in /proc/slabinfo to identify this cache.
  * @size: The size of objects to be created in this cache.
  * @align: The required alignment for the objects.
@@ -407,9 +407,9 @@ EXPORT_SYMBOL(kmem_cache_create_usercopy);
  *
  * Return: a pointer to the cache on success, NULL on failure.
  */
-struct kmem_cache *
-kmem_cache_create(const char *name, unsigned int size, unsigned int align,
-		slab_flags_t flags, void (*ctor)(void *))
+struct kmem_cache *__kmem_cache_create(const char *name, unsigned int size,
+				       unsigned int align, slab_flags_t flags,
+				       void (*ctor)(void *))
 {
 	struct kmem_cache_args kmem_args = {
 		.align	= align,
@@ -418,7 +418,7 @@ kmem_cache_create(const char *name, unsigned int size, unsigned int align,
 
 	return __kmem_cache_create_args(name, size, &kmem_args, flags);
 }
-EXPORT_SYMBOL(kmem_cache_create);
+EXPORT_SYMBOL(__kmem_cache_create);
 
 /**
  * kmem_cache_create_rcu - Create a SLAB_TYPESAFE_BY_RCU cache.
-- 
2.45.2