[RFC,0/6] slab: add kmem_cache_setup()

Message

Christian Brauner Sept. 2, 2024, 3:31 p.m. UTC

Hey,

As discussed last week the various kmem_cache_*() functions should be
replaced by a unified function that is based around a struct, with only
the basic parameters passed separately.

Vlastimil already hinted that he would like to keep core parameters out
of the struct: name, object size, and flags. I personally don't care
much and would not object to moving everything into the struct but
that's a matter of taste and I yield that decision power to the
maintainer.

Here's an RFC built for a kmem_cache_setup() based on struct
kmem_cache_args.

The choice of name is somewhat forced as kmem_cache_create() is taken
and the only way to reuse it would be to replace all users in one go.
Or to do a global sed/kmem_cache_create()/kmem_cache_create2()/g. And
then introduce kmem_cache_setup(). That doesn't strike me as a viable
option.

If we really cared about the *_create() suffix then an alternative might
be to do a sed/kmem_cache_setup()/kmem_cache_create()/g after every user
in the kernel is ported. I honestly don't think that's worth it but I
wanted to at least mention it to highlight the fact that this might lead
to a naming compromise.

From a cursory grep (and not excluding Documentation mentions) we will
have to replace 44 kmem_cache_create_usercopy() calls and about 463
kmem_cache_create() calls which makes for a bit above 500 calls to port
to kmem_cache_setup(). That'll probably be good work for people getting
into kernel development.

Anyway, I wanted to get an RFC out to get some rough agreement on what
the struct should look like, get some bikeshedding on the name done, and
what else needs to be massaged as part of this. I think that
cache_create() is the deepest we should stuff struct kmem_cache_args
into the bowels of slab.

Signed-off-by: Christian Brauner <[e-mail]>
---
Christian Brauner (6):
      slab: introduce kmem_cache_setup()
      slab: port KMEM_CACHE() to kmem_cache_setup()
      slab: port KMEM_CACHE_USERCOPY() to kmem_cache_setup()
      file: port to kmem_cache_setup()
      slab: remove kmem_cache_create_rcu()
      io_uring: port to kmem_cache_setup()

 Documentation/core-api/memory-allocation.rst |  10 +-
 fs/file_table.c                              |  12 ++-
 include/linux/slab.h                         |  51 ++++++++---
 io_uring/io_uring.c                          |  15 +--
 mm/slab_common.c                             | 132 ++++++++++++---------------
 5 files changed, 121 insertions(+), 99 deletions(-)
---
base-commit: 6e016babce7c845ed015da25c7a097fa3482d95a
change-id: 20240902-work-kmem_cache_args-e9760972c7d4

Comments

Vlastimil Babka Sept. 2, 2024, 6:15 p.m. UTC | #1

On 9/2/24 17:31, Christian Brauner wrote:
> Hey,
> 
> As discussed last week the various kmem_cache_*() functions should be
> replaced by a unified function that is based around a struct, with only
> the basic parameters passed separately.
> 
> Vlastimil already hinted that he would like to keep core parameters out
> of the struct: name, object size, and flags. I personally don't care
> much and would not object to moving everything into the struct but
> that's a matter of taste and I yield that decision power to the
> maintainer.

Yeah the reason I suggested that is the new struct contains rarely needed
arguments, so most usages won't have to instantiate it and thus look a bit
nicer.

> Here's an RFC built for a kmem_cache_setup() based on struct
> kmem_cache_args.
> 
> The choice of name is somewhat forced as kmem_cache_create() is taken
> and the only way to reuse it would be to replace all users in one go.
> Or to do a global sed/kmem_cache_create()/kmem_cache_create2()/g. And
> then introduce kmem_cache_setup(). That doesn't strike me as a viable
> option.
> 
> If we really cared about the *_create() suffix then an alternative might
> be to do a sed/kmem_cache_setup()/kmem_cache_create()/g after every user
> in the kernel is ported. I honestly don't think that's worth it but I
> wanted to at least mention it to highlight the fact that this might lead
> to a naming compromise.

I think using macros would allow us to have kmem_cache_create() in both the
current variant (5 args) and new one (4 args) at the same time? But that's
also not ideal so perhaps viable only if we really decided to convert
everything sooner than later and drop that temporary compatibility layer.

But perhaps if we're converting, it should be mainly to KMEM_CACHE() as it
handles alignment.

> From a cursory grep (and not excluding Documentation mentions) we will
> have to replace 44 kmem_cache_create_usercopy() calls and about 463
> kmem_cache_create() calls which makes for a bit above 500 calls to port
> to kmem_cache_setup(). That'll probably be good work for people getting
> into kernel development.
> 
> Anyway, I wanted to get an RFC out to get some rough agreement on what
> the struct should look like, get some bikeshedding on the name done, and
> what else needs to be massaged as part of this. I think that
> cache_create() is the deepest we should stuff struct kmem_cache_args
> into the bowels of slab.

Well, if you wanted to be more adventurous... we could pass it also to
__kmem_cache_create(), then remove kmem_cache_open() (move the code to its
only caller __kmem_cache_create(), probably another thing not cleaned up
after SLAB removal).

And then also pass it to calculate_sizes() and at that point
rcu_freeptr_offset can be removed from struct kmem_cache, because we just
use the value from kmem_cache_args to calculate s->offset and then we can
forget that it was requested specifically.

> Signed-off-by: Christian Brauner <[e-mail]>
> ---
> Christian Brauner (6):
>       slab: introduce kmem_cache_setup()
>       slab: port KMEM_CACHE() to kmem_cache_setup()
>       slab: port KMEM_CACHE_USERCOPY() to kmem_cache_setup()
>       file: port to kmem_cache_setup()
>       slab: remove kmem_cache_create_rcu()
>       io_uring: port to kmem_cache_setup()
> 
>  Documentation/core-api/memory-allocation.rst |  10 +-
>  fs/file_table.c                              |  12 ++-
>  include/linux/slab.h                         |  51 ++++++++---
>  io_uring/io_uring.c                          |  15 +--
>  mm/slab_common.c                             | 132 ++++++++++++---------------
>  5 files changed, 121 insertions(+), 99 deletions(-)
> ---
> base-commit: 6e016babce7c845ed015da25c7a097fa3482d95a
> change-id: 20240902-work-kmem_cache_args-e9760972c7d4
>