[PATCH v2 0/3] fs,mm: add kmem_cache_create_rcu()

[PATCH v2 0/3] fs,mm: add kmem_cache_create_rcu()

[Christian Brauner]

When a kmem cache is created with SLAB_TYPESAFE_BY_RCU the free pointer
must be located outside of the object because we don't know what part of
the memory can safely be overwritten as it may be needed to prevent
object recycling.

That has the consequence that SLAB_TYPESAFE_BY_RCU may end up adding a
new cacheline. This is the case for .e.g, struct file. After having it
shrunk down by 40 bytes and having it fit in three cachelines we still
have SLAB_TYPESAFE_BY_RCU adding a fourth cacheline because it needs to
accomodate the free pointer and is hardware cacheline aligned.

I tried to find ways to rectify this as struct file is pretty much
everywhere and having it use less memory is a good thing. So here's a
proposal.

I was hoping to get something to this effect into v6.12.

If we really want to switch to a struct to pass kmem_cache parameters I
can do the preparatory patch to convert all kmem_cache_create() and
kmem_cache_create_usercopy() callers to use a struct for initialization
of course. I can do this as a preparatory work or as follow-up work to
this series. Thoughts?

Thanks!
Christian

---
---
base-commit: 766508e7e2c5075eb744cb29b8cef6fa835b0344
change-id: 20240827-work-kmem_cache-rcu-f97e35600cc6

[PATCH v2 1/3] mm: remove unused root_cache argument

[Christian Brauner]

That argument is unused so remove it.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 mm/slab_common.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index 40b582a014b8..c8dd7e08c5f6 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -204,8 +204,7 @@ struct kmem_cache *find_mergeable(unsigned int size, unsigned int align,
 static struct kmem_cache *create_cache(const char *name,
 		unsigned int object_size, unsigned int align,
 		slab_flags_t flags, unsigned int useroffset,
-		unsigned int usersize, void (*ctor)(void *),
-		struct kmem_cache *root_cache)
+		unsigned int usersize, void (*ctor)(void *))
 {
 	struct kmem_cache *s;
 	int err;
@@ -334,7 +333,7 @@ kmem_cache_create_usercopy(const char *name,
 
 	s = create_cache(cache_name, size,
 			 calculate_alignment(flags, align, size),
-			 flags, useroffset, usersize, ctor, NULL);
+			 flags, useroffset, usersize, ctor);
 	if (IS_ERR(s)) {
 		err = PTR_ERR(s);
 		kfree_const(cache_name);

-- 
2.45.2

[PATCH v2 2/3] mm: add kmem_cache_create_rcu()

[Christian Brauner]

When a kmem cache is created with SLAB_TYPESAFE_BY_RCU the free pointer
must be located outside of the object because we don't know what part of
the memory can safely be overwritten as it may be needed to prevent
object recycling.

That has the consequence that SLAB_TYPESAFE_BY_RCU may end up adding a
new cacheline. This is the case for .e.g, struct file. After having it
shrunk down by 40 bytes and having it fit in three cachelines we still
have SLAB_TYPESAFE_BY_RCU adding a fourth cacheline because it needs to
accomodate the free pointer and is hardware cacheline aligned.

I tried to find ways to rectify this as struct file is pretty much
everywhere and having it use less memory is a good thing. So here's a
proposal.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 include/linux/slab.h |   9 ++++
 mm/slab.h            |   1 +
 mm/slab_common.c     | 133 ++++++++++++++++++++++++++++++++++++---------------
 mm/slub.c            |  17 ++++---
 4 files changed, 114 insertions(+), 46 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index eb2bf4629157..5b2da2cf31a8 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -212,6 +212,12 @@ enum _slab_flag_bits {
 #define SLAB_NO_OBJ_EXT		__SLAB_FLAG_UNUSED
 #endif
 
+/*
+ * freeptr_t represents a SLUB freelist pointer, which might be encoded
+ * and not dereferenceable if CONFIG_SLAB_FREELIST_HARDENED is enabled.
+ */
+typedef struct { unsigned long v; } freeptr_t;
+
 /*
  * ZERO_SIZE_PTR will be returned for zero sized kmalloc requests.
  *
@@ -242,6 +248,9 @@ struct kmem_cache *kmem_cache_create_usercopy(const char *name,
 			slab_flags_t flags,
 			unsigned int useroffset, unsigned int usersize,
 			void (*ctor)(void *));
+struct kmem_cache *kmem_cache_create_rcu(const char *name, unsigned int size,
+					 unsigned int freeptr_offset,
+					 slab_flags_t flags);
 void kmem_cache_destroy(struct kmem_cache *s);
 int kmem_cache_shrink(struct kmem_cache *s);
 
diff --git a/mm/slab.h b/mm/slab.h
index dcdb56b8e7f5..b05512a14f07 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -261,6 +261,7 @@ struct kmem_cache {
 	unsigned int object_size;	/* Object size without metadata */
 	struct reciprocal_value reciprocal_size;
 	unsigned int offset;		/* Free pointer offset */
+	unsigned int rcu_freeptr_offset; /* Specific free pointer requested */
 #ifdef CONFIG_SLUB_CPU_PARTIAL
 	/* Number of per cpu partial objects to keep around */
 	unsigned int cpu_partial;
diff --git a/mm/slab_common.c b/mm/slab_common.c
index c8dd7e08c5f6..c4beff642fff 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -202,9 +202,10 @@ struct kmem_cache *find_mergeable(unsigned int size, unsigned int align,
 }
 
 static struct kmem_cache *create_cache(const char *name,
-		unsigned int object_size, unsigned int align,
-		slab_flags_t flags, unsigned int useroffset,
-		unsigned int usersize, void (*ctor)(void *))
+		unsigned int object_size, unsigned int freeptr_offset,
+		unsigned int align, slab_flags_t flags,
+		unsigned int useroffset, unsigned int usersize,
+		void (*ctor)(void *))
 {
 	struct kmem_cache *s;
 	int err;
@@ -212,6 +213,12 @@ static struct kmem_cache *create_cache(const char *name,
 	if (WARN_ON(useroffset + usersize > object_size))
 		useroffset = usersize = 0;
 
+	err = -EINVAL;
+	if (freeptr_offset < UINT_MAX &&
+	    (freeptr_offset >= object_size ||
+	     (freeptr_offset && !(flags & SLAB_TYPESAFE_BY_RCU))))
+		goto out;
+
 	err = -ENOMEM;
 	s = kmem_cache_zalloc(kmem_cache, GFP_KERNEL);
 	if (!s)
@@ -219,13 +226,13 @@ static struct kmem_cache *create_cache(const char *name,
 
 	s->name = name;
 	s->size = s->object_size = object_size;
+	s->rcu_freeptr_offset = freeptr_offset;
 	s->align = align;
 	s->ctor = ctor;
 #ifdef CONFIG_HARDENED_USERCOPY
 	s->useroffset = useroffset;
 	s->usersize = usersize;
 #endif
-
 	err = __kmem_cache_create(s, flags);
 	if (err)
 		goto out_free_cache;
@@ -240,38 +247,10 @@ static struct kmem_cache *create_cache(const char *name,
 	return ERR_PTR(err);
 }
 
-/**
- * kmem_cache_create_usercopy - Create a cache with a region suitable
- * for copying to userspace
- * @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.
- * @flags: SLAB flags
- * @useroffset: Usercopy region offset
- * @usersize: Usercopy region size
- * @ctor: A constructor for the objects.
- *
- * Cannot be called within a interrupt, but can be interrupted.
- * The @ctor is run when new pages are allocated by the cache.
- *
- * The flags are
- *
- * %SLAB_POISON - Poison the slab with a known test pattern (a5a5a5a5)
- * to catch references to uninitialised memory.
- *
- * %SLAB_RED_ZONE - Insert `Red` zones around the allocated memory to check
- * for buffer overruns.
- *
- * %SLAB_HWCACHE_ALIGN - Align the objects in this cache to a hardware
- * cacheline.  This can be beneficial if you're counting cycles as closely
- * as davem.
- *
- * Return: a pointer to the cache on success, NULL on failure.
- */
-struct kmem_cache *
-kmem_cache_create_usercopy(const char *name,
-		  unsigned int size, unsigned int align,
-		  slab_flags_t flags,
+static struct kmem_cache *
+do_kmem_cache_create_usercopy(const char *name,
+		  unsigned int size, unsigned int freeptr_offset,
+		  unsigned int align, slab_flags_t flags,
 		  unsigned int useroffset, unsigned int usersize,
 		  void (*ctor)(void *))
 {
@@ -331,7 +310,7 @@ kmem_cache_create_usercopy(const char *name,
 		goto out_unlock;
 	}
 
-	s = create_cache(cache_name, size,
+	s = create_cache(cache_name, size, freeptr_offset,
 			 calculate_alignment(flags, align, size),
 			 flags, useroffset, usersize, ctor);
 	if (IS_ERR(s)) {
@@ -355,6 +334,45 @@ kmem_cache_create_usercopy(const char *name,
 	}
 	return s;
 }
+
+/**
+ * kmem_cache_create_usercopy - Create a cache with a region suitable
+ * for copying to userspace
+ * @name: A string which is used in /proc/slabinfo to identify this cache.
+ * @size: The size of objects to be created in this cache.
+ * @freeptr_offset: Custom offset for the free pointer in RCU caches
+ * @align: The required alignment for the objects.
+ * @flags: SLAB flags
+ * @useroffset: Usercopy region offset
+ * @usersize: Usercopy region size
+ * @ctor: A constructor for the objects.
+ *
+ * Cannot be called within a interrupt, but can be interrupted.
+ * The @ctor is run when new pages are allocated by the cache.
+ *
+ * The flags are
+ *
+ * %SLAB_POISON - Poison the slab with a known test pattern (a5a5a5a5)
+ * to catch references to uninitialised memory.
+ *
+ * %SLAB_RED_ZONE - Insert `Red` zones around the allocated memory to check
+ * for buffer overruns.
+ *
+ * %SLAB_HWCACHE_ALIGN - Align the objects in this cache to a hardware
+ * cacheline.  This can be beneficial if you're counting cycles as closely
+ * as davem.
+ *
+ * Return: a pointer to the cache on success, NULL on failure.
+ */
+struct kmem_cache *
+kmem_cache_create_usercopy(const char *name, unsigned int size,
+			   unsigned int align, slab_flags_t flags,
+			   unsigned int useroffset, unsigned int usersize,
+			   void (*ctor)(void *))
+{
+	return do_kmem_cache_create_usercopy(name, size, UINT_MAX, align, flags,
+					     useroffset, usersize, ctor);
+}
 EXPORT_SYMBOL(kmem_cache_create_usercopy);
 
 /**
@@ -386,11 +404,48 @@ struct kmem_cache *
 kmem_cache_create(const char *name, unsigned int size, unsigned int align,
 		slab_flags_t flags, void (*ctor)(void *))
 {
-	return kmem_cache_create_usercopy(name, size, align, flags, 0, 0,
-					  ctor);
+	return do_kmem_cache_create_usercopy(name, size, UINT_MAX, align, flags,
+					     0, 0, ctor);
 }
 EXPORT_SYMBOL(kmem_cache_create);
 
+/**
+ * kmem_cache_create_rcu - Create a SLAB_TYPESAFE_BY_RCU 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.
+ * @freeptr_offset: The offset into the memory to the free pointer
+ * @flags: SLAB flags
+ *
+ * Cannot be called within a interrupt, but can be interrupted.
+ * The @ctor is run when new pages are allocated by the cache.
+ *
+ * See kmem_cache_create() for an explanation of possible @flags.
+ *
+ * By default SLAB_TYPESAFE_BY_RCU caches place free pointer outside of
+ * the object. This might cause the object to grow in size. Callers that
+ * have a reason to avoid this can specify a custom offset in their
+ * struct where the free pointer will be placed.
+ *
+ * Note that placing the free pointer inside the object requires the
+ * caller to ensure that no fields are invalidated that are required to
+ * guard against object recycling (See SLAB_TYPESAFE_BY_RCU for
+ * details.).
+ *
+ * Using zero as a value for @freeptr_offset is valid. To request no offset
+ * UINT_MAX can be specified.
+ *
+ * Return: a pointer to the cache on success, NULL on failure.
+ */
+struct kmem_cache *kmem_cache_create_rcu(const char *name, unsigned int size,
+					 unsigned int freeptr_offset,
+					 slab_flags_t flags)
+{
+	return do_kmem_cache_create_usercopy(name, size, freeptr_offset, 0,
+					     flags | SLAB_TYPESAFE_BY_RCU, 0, 0,
+					     NULL);
+}
+EXPORT_SYMBOL(kmem_cache_create_rcu);
+
 static struct kmem_cache *kmem_buckets_cache __ro_after_init;
 
 /**
diff --git a/mm/slub.c b/mm/slub.c
index c9d8a2497fd6..b75f320e3963 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -465,12 +465,6 @@ static struct workqueue_struct *flushwq;
  * 			Core slab cache functions
  *******************************************************************/
 
-/*
- * freeptr_t represents a SLUB freelist pointer, which might be encoded
- * and not dereferenceable if CONFIG_SLAB_FREELIST_HARDENED is enabled.
- */
-typedef struct { unsigned long v; } freeptr_t;
-
 /*
  * Returns freelist pointer (ptr). With hardening, this is obfuscated
  * with an XOR of the address where the pointer is held and a per-cache
@@ -5144,6 +5138,12 @@ static void set_cpu_partial(struct kmem_cache *s)
 #endif
 }
 
+/* Was a valid freeptr offset requested? */
+static inline bool has_freeptr_offset(const struct kmem_cache *s)
+{
+	return s->rcu_freeptr_offset != UINT_MAX;
+}
+
 /*
  * calculate_sizes() determines the order and the distribution of data within
  * a slab object.
@@ -5189,7 +5189,8 @@ static int calculate_sizes(struct kmem_cache *s)
 	 */
 	s->inuse = size;
 
-	if ((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) || s->ctor ||
+	if (((flags & SLAB_TYPESAFE_BY_RCU) && !has_freeptr_offset(s)) ||
+	    (flags & SLAB_POISON) || s->ctor ||
 	    ((flags & SLAB_RED_ZONE) &&
 	     (s->object_size < sizeof(void *) || slub_debug_orig_size(s)))) {
 		/*
@@ -5210,6 +5211,8 @@ static int calculate_sizes(struct kmem_cache *s)
 		 */
 		s->offset = size;
 		size += sizeof(void *);
+	} else if ((flags & SLAB_TYPESAFE_BY_RCU) && has_freeptr_offset(s)) {
+		s->offset = s->rcu_freeptr_offset;
 	} else {
 		/*
 		 * Store freelist pointer near middle of object to keep

-- 
2.45.2

Re: [PATCH v2 2/3] mm: add kmem_cache_create_rcu()

[Linus Torvalds]

On Wed, 28 Aug 2024 at 04:07, Christian Brauner <brauner@kernel.org> wrote:
>
>  static struct kmem_cache *create_cache(const char *name,
> -               unsigned int object_size, unsigned int align,
> -               slab_flags_t flags, unsigned int useroffset,
> -               unsigned int usersize, void (*ctor)(void *))
> +               unsigned int object_size, unsigned int freeptr_offset,
> +               unsigned int align, slab_flags_t flags,
> +               unsigned int useroffset, unsigned int usersize,
> +               void (*ctor)(void *))

I actually think it would be nicer at this point to have a "kmem_cache
create structure", and not 8 individual arguments.

But this approach looks fine to me even without that.

                  Linus

Re: [PATCH v2 2/3] mm: add kmem_cache_create_rcu()

[Vlastimil Babka]

On 8/27/24 17:59, Christian Brauner wrote:
> When a kmem cache is created with SLAB_TYPESAFE_BY_RCU the free pointer
> must be located outside of the object because we don't know what part of
> the memory can safely be overwritten as it may be needed to prevent
> object recycling.
> 
> That has the consequence that SLAB_TYPESAFE_BY_RCU may end up adding a
> new cacheline. This is the case for .e.g, struct file. After having it
> shrunk down by 40 bytes and having it fit in three cachelines we still
> have SLAB_TYPESAFE_BY_RCU adding a fourth cacheline because it needs to
> accomodate the free pointer and is hardware cacheline aligned.
> 
> I tried to find ways to rectify this as struct file is pretty much
> everywhere and having it use less memory is a good thing. So here's a
> proposal.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

So logistically patch 3 needs stuff in the vfs tree and having 1+2 in slab
tree and 3 in vfs that depends on 1+2 elsewhere is infeasible, so it will be
easiest for whole series to be in vfs, right?

> ---
>  include/linux/slab.h |   9 ++++
>  mm/slab.h            |   1 +
>  mm/slab_common.c     | 133 ++++++++++++++++++++++++++++++++++++---------------
>  mm/slub.c            |  17 ++++---
>  4 files changed, 114 insertions(+), 46 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index eb2bf4629157..5b2da2cf31a8 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -212,6 +212,12 @@ enum _slab_flag_bits {
>  #define SLAB_NO_OBJ_EXT		__SLAB_FLAG_UNUSED
>  #endif
>  
> +/*
> + * freeptr_t represents a SLUB freelist pointer, which might be encoded
> + * and not dereferenceable if CONFIG_SLAB_FREELIST_HARDENED is enabled.
> + */
> +typedef struct { unsigned long v; } freeptr_t;
> +
>  /*
>   * ZERO_SIZE_PTR will be returned for zero sized kmalloc requests.
>   *
> @@ -242,6 +248,9 @@ struct kmem_cache *kmem_cache_create_usercopy(const char *name,
>  			slab_flags_t flags,
>  			unsigned int useroffset, unsigned int usersize,
>  			void (*ctor)(void *));
> +struct kmem_cache *kmem_cache_create_rcu(const char *name, unsigned int size,
> +					 unsigned int freeptr_offset,
> +					 slab_flags_t flags);
>  void kmem_cache_destroy(struct kmem_cache *s);
>  int kmem_cache_shrink(struct kmem_cache *s);
>  
> diff --git a/mm/slab.h b/mm/slab.h
> index dcdb56b8e7f5..b05512a14f07 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -261,6 +261,7 @@ struct kmem_cache {
>  	unsigned int object_size;	/* Object size without metadata */
>  	struct reciprocal_value reciprocal_size;
>  	unsigned int offset;		/* Free pointer offset */
> +	unsigned int rcu_freeptr_offset; /* Specific free pointer requested */

More precisely something like:

				  Specific offset requested (if not
				  UINT_MAX)

?

>  #ifdef CONFIG_SLUB_CPU_PARTIAL
>  	/* Number of per cpu partial objects to keep around */
>  	unsigned int cpu_partial;
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index c8dd7e08c5f6..c4beff642fff 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -202,9 +202,10 @@ struct kmem_cache *find_mergeable(unsigned int size, unsigned int align,
>  }
>  
>  static struct kmem_cache *create_cache(const char *name,
> -		unsigned int object_size, unsigned int align,
> -		slab_flags_t flags, unsigned int useroffset,
> -		unsigned int usersize, void (*ctor)(void *))
> +		unsigned int object_size, unsigned int freeptr_offset,
> +		unsigned int align, slab_flags_t flags,
> +		unsigned int useroffset, unsigned int usersize,
> +		void (*ctor)(void *))
>  {
>  	struct kmem_cache *s;
>  	int err;
> @@ -212,6 +213,12 @@ static struct kmem_cache *create_cache(const char *name,
>  	if (WARN_ON(useroffset + usersize > object_size))
>  		useroffset = usersize = 0;
>  
> +	err = -EINVAL;
> +	if (freeptr_offset < UINT_MAX &&

freeptr_offset != UINT_MAX to be more obvious and match has_freeptr_offset() ?

> +	    (freeptr_offset >= object_size ||
> +	     (freeptr_offset && !(flags & SLAB_TYPESAFE_BY_RCU))))

and here drop the "freeptr_offset &&" as zero is a valid value

instead we could want alignment to sizeof(freeptr_t) if we were paranoid?

> +		goto out;

The rest seems good to me now.

Re: [PATCH v2 2/3] mm: add kmem_cache_create_rcu()

[Christian Brauner]

On Tue, Aug 27, 2024 at 11:10:10PM GMT, Vlastimil Babka wrote:
> On 8/27/24 17:59, Christian Brauner wrote:
> > When a kmem cache is created with SLAB_TYPESAFE_BY_RCU the free pointer
> > must be located outside of the object because we don't know what part of
> > the memory can safely be overwritten as it may be needed to prevent
> > object recycling.
> > 
> > That has the consequence that SLAB_TYPESAFE_BY_RCU may end up adding a
> > new cacheline. This is the case for .e.g, struct file. After having it
> > shrunk down by 40 bytes and having it fit in three cachelines we still
> > have SLAB_TYPESAFE_BY_RCU adding a fourth cacheline because it needs to
> > accomodate the free pointer and is hardware cacheline aligned.
> > 
> > I tried to find ways to rectify this as struct file is pretty much
> > everywhere and having it use less memory is a good thing. So here's a
> > proposal.
> > 
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> 
> So logistically patch 3 needs stuff in the vfs tree and having 1+2 in slab
> tree and 3 in vfs that depends on 1+2 elsewhere is infeasible, so it will be
> easiest for whole series to be in vfs, right?

Yeah, that's fine by me.

> 
> > ---
> >  include/linux/slab.h |   9 ++++
> >  mm/slab.h            |   1 +
> >  mm/slab_common.c     | 133 ++++++++++++++++++++++++++++++++++++---------------
> >  mm/slub.c            |  17 ++++---
> >  4 files changed, 114 insertions(+), 46 deletions(-)
> > 
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index eb2bf4629157..5b2da2cf31a8 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -212,6 +212,12 @@ enum _slab_flag_bits {
> >  #define SLAB_NO_OBJ_EXT		__SLAB_FLAG_UNUSED
> >  #endif
> >  
> > +/*
> > + * freeptr_t represents a SLUB freelist pointer, which might be encoded
> > + * and not dereferenceable if CONFIG_SLAB_FREELIST_HARDENED is enabled.
> > + */
> > +typedef struct { unsigned long v; } freeptr_t;
> > +
> >  /*
> >   * ZERO_SIZE_PTR will be returned for zero sized kmalloc requests.
> >   *
> > @@ -242,6 +248,9 @@ struct kmem_cache *kmem_cache_create_usercopy(const char *name,
> >  			slab_flags_t flags,
> >  			unsigned int useroffset, unsigned int usersize,
> >  			void (*ctor)(void *));
> > +struct kmem_cache *kmem_cache_create_rcu(const char *name, unsigned int size,
> > +					 unsigned int freeptr_offset,
> > +					 slab_flags_t flags);
> >  void kmem_cache_destroy(struct kmem_cache *s);
> >  int kmem_cache_shrink(struct kmem_cache *s);
> >  
> > diff --git a/mm/slab.h b/mm/slab.h
> > index dcdb56b8e7f5..b05512a14f07 100644
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -261,6 +261,7 @@ struct kmem_cache {
> >  	unsigned int object_size;	/* Object size without metadata */
> >  	struct reciprocal_value reciprocal_size;
> >  	unsigned int offset;		/* Free pointer offset */
> > +	unsigned int rcu_freeptr_offset; /* Specific free pointer requested */
> 
> More precisely something like:
> 
> 				  Specific offset requested (if not
> 				  UINT_MAX)

Yep, added that.

> 
> ?
> 
> >  #ifdef CONFIG_SLUB_CPU_PARTIAL
> >  	/* Number of per cpu partial objects to keep around */
> >  	unsigned int cpu_partial;
> > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > index c8dd7e08c5f6..c4beff642fff 100644
> > --- a/mm/slab_common.c
> > +++ b/mm/slab_common.c
> > @@ -202,9 +202,10 @@ struct kmem_cache *find_mergeable(unsigned int size, unsigned int align,
> >  }
> >  
> >  static struct kmem_cache *create_cache(const char *name,
> > -		unsigned int object_size, unsigned int align,
> > -		slab_flags_t flags, unsigned int useroffset,
> > -		unsigned int usersize, void (*ctor)(void *))
> > +		unsigned int object_size, unsigned int freeptr_offset,
> > +		unsigned int align, slab_flags_t flags,
> > +		unsigned int useroffset, unsigned int usersize,
> > +		void (*ctor)(void *))
> >  {
> >  	struct kmem_cache *s;
> >  	int err;
> > @@ -212,6 +213,12 @@ static struct kmem_cache *create_cache(const char *name,
> >  	if (WARN_ON(useroffset + usersize > object_size))
> >  		useroffset = usersize = 0;
> >  
> > +	err = -EINVAL;
> > +	if (freeptr_offset < UINT_MAX &&
> 
> freeptr_offset != UINT_MAX to be more obvious and match has_freeptr_offset() ?

Done.

> 
> > +	    (freeptr_offset >= object_size ||
> > +	     (freeptr_offset && !(flags & SLAB_TYPESAFE_BY_RCU))))
> 
> and here drop the "freeptr_offset &&" as zero is a valid value

Yes, thank you.

> 
> instead we could want alignment to sizeof(freeptr_t) if we were paranoid?

Added a check for that.

> 
> > +		goto out;
> 
> The rest seems good to me now.

Thanks for the review! v3 incoming

[PATCH v2 3/3] fs: use kmem_cache_create_rcu()

[Christian Brauner]

Switch to the new kmem_cache_create_rcu() helper which allows us to use
a custom free pointer offset avoiding the need to have an external free
pointer which would grow struct file behind our backs.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/file_table.c    | 6 +++---
 include/linux/fs.h | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 694199a1a966..83d5ac1fadc0 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -514,9 +514,9 @@ EXPORT_SYMBOL(__fput_sync);
 
 void __init files_init(void)
 {
-	filp_cachep = kmem_cache_create("filp", sizeof(struct file), 0,
-				SLAB_TYPESAFE_BY_RCU | SLAB_HWCACHE_ALIGN |
-				SLAB_PANIC | SLAB_ACCOUNT, NULL);
+	filp_cachep = kmem_cache_create_rcu("filp", sizeof(struct file),
+				offsetof(struct file, f_freeptr),
+				SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
 	percpu_counter_init(&nr_files, 0, GFP_KERNEL);
 }
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 61097a9cf317..12a72f162da7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1057,6 +1057,7 @@ struct file {
 		struct callback_head	f_task_work;
 		struct llist_node	f_llist;
 		struct file_ra_state	f_ra;
+		freeptr_t		f_freeptr;
 	};
 	/* --- cacheline 3 boundary (192 bytes) --- */
 } __randomize_layout

-- 
2.45.2

Re: [PATCH v2 0/3] fs,mm: add kmem_cache_create_rcu()

[Christian Brauner]

On Tue, Aug 27, 2024 at 05:59:41PM GMT, Christian Brauner wrote:
> When a kmem cache is created with SLAB_TYPESAFE_BY_RCU the free pointer
> must be located outside of the object because we don't know what part of
> the memory can safely be overwritten as it may be needed to prevent
> object recycling.
> 
> That has the consequence that SLAB_TYPESAFE_BY_RCU may end up adding a
> new cacheline. This is the case for .e.g, struct file. After having it
> shrunk down by 40 bytes and having it fit in three cachelines we still
> have SLAB_TYPESAFE_BY_RCU adding a fourth cacheline because it needs to
> accomodate the free pointer and is hardware cacheline aligned.
> 
> I tried to find ways to rectify this as struct file is pretty much
> everywhere and having it use less memory is a good thing. So here's a
> proposal.
> 
> I was hoping to get something to this effect into v6.12.
> 
> If we really want to switch to a struct to pass kmem_cache parameters I
> can do the preparatory patch to convert all kmem_cache_create() and
> kmem_cache_create_usercopy() callers to use a struct for initialization
> of course. I can do this as a preparatory work or as follow-up work to
> this series. Thoughts?

So one thing I can do is to add:

struct kmem_cache_args {
	.freeptr_offset,
	.useroffset,
	.flags,
	.name,
};

accompanied by:

int kmem_create_cache(struct kmem_cache_args *args);

and then switch both the filp cache and Jens' io_kiocb cache over to use
these two helpers. Then we can convert other callers one by one.

@Vlastimil, @Jens, @Linus what do you think?

Re: [PATCH v2 0/3] fs,mm: add kmem_cache_create_rcu()

[Vlastimil Babka]

On 8/27/24 18:05, Christian Brauner wrote:
> On Tue, Aug 27, 2024 at 05:59:41PM GMT, Christian Brauner wrote:
>> When a kmem cache is created with SLAB_TYPESAFE_BY_RCU the free pointer
>> must be located outside of the object because we don't know what part of
>> the memory can safely be overwritten as it may be needed to prevent
>> object recycling.
>> 
>> That has the consequence that SLAB_TYPESAFE_BY_RCU may end up adding a
>> new cacheline. This is the case for .e.g, struct file. After having it
>> shrunk down by 40 bytes and having it fit in three cachelines we still
>> have SLAB_TYPESAFE_BY_RCU adding a fourth cacheline because it needs to
>> accomodate the free pointer and is hardware cacheline aligned.
>> 
>> I tried to find ways to rectify this as struct file is pretty much
>> everywhere and having it use less memory is a good thing. So here's a
>> proposal.
>> 
>> I was hoping to get something to this effect into v6.12.
>> 
>> If we really want to switch to a struct to pass kmem_cache parameters I
>> can do the preparatory patch to convert all kmem_cache_create() and
>> kmem_cache_create_usercopy() callers to use a struct for initialization
>> of course. I can do this as a preparatory work or as follow-up work to
>> this series. Thoughts?
> 
> So one thing I can do is to add:
> 
> struct kmem_cache_args {
> 	.freeptr_offset,
> 	.useroffset,
> 	.flags,
> 	.name,
> };

Hm basically everyone uses name, size and some flags, so how about we leave
those as direct parameters and args is for the rest, and in most cases would
be NULL.

> accompanied by:
> 
> int kmem_create_cache(struct kmem_cache_args *args);

I think we can't reuse the name with different parameters as long the old
one exists?

> and then switch both the filp cache and Jens' io_kiocb cache over to use
> these two helpers. Then we can convert other callers one by one.
> 
> @Vlastimil, @Jens, @Linus what do you think?

In the other thread you said it's best to leave such refactoring to
maintainers and I agree and don't ask you to do the cleanup in order to get
what you need (and we don't need to rush it either).

Re: [PATCH v2 0/3] fs,mm: add kmem_cache_create_rcu()

[Christian Brauner]

> > @Vlastimil, @Jens, @Linus what do you think?
> 
> In the other thread you said it's best to leave such refactoring to
> maintainers and I agree and don't ask you to do the cleanup in order to get
> what you need (and we don't need to rush it either).

I mainly didn't want to wait for that huge refactor to be done before we
get a workable solution for v6.12 since the patch isn't all that huge.

But I'm not shying away from cleanup work I'm the cause of.

So I'm happy to do it for v6.13 as a follow-up but I also don't want to
steal this away from you because it'll probably be a fun task to get to
port everything to struct kmem_cache_args and clean that all up.

Re: [PATCH v2 0/3] fs,mm: add kmem_cache_create_rcu()

[Vlastimil Babka]

On 8/28/24 14:18, Christian Brauner wrote:
>> > @Vlastimil, @Jens, @Linus what do you think?
>> 
>> In the other thread you said it's best to leave such refactoring to
>> maintainers and I agree and don't ask you to do the cleanup in order to get
>> what you need (and we don't need to rush it either).
> 
> I mainly didn't want to wait for that huge refactor to be done before we
> get a workable solution for v6.12 since the patch isn't all that huge.
> 
> But I'm not shying away from cleanup work I'm the cause of.
> 
> So I'm happy to do it for v6.13 as a follow-up but I also don't want to
> steal this away from you because it'll probably be a fun task to get to
> port everything to struct kmem_cache_args and clean that all up.

So if you want, be my guest :) Yeah could be a fun task, but I'll probably
be too busy for a while now.