Re: [PATCH v2 12/15] slab: create kmem_cache_create() compatibility layer

[Christian Brauner <brauner@kernel.org>]

On Wed, Sep 04, 2024 at 06:11:10PM GMT, Mike Rapoport wrote:
> On Wed, Sep 04, 2024 at 04:44:03PM +0200, Christian Brauner wrote:
> > On Wed, Sep 04, 2024 at 03:33:30PM GMT, Vlastimil Babka wrote:
> > > On 9/4/24 13:38, Christian Brauner wrote:
> > > > On Wed, Sep 04, 2024 at 12:50:28PM GMT, Vlastimil Babka wrote:
> > > >> On 9/4/24 11:45, Christian Brauner wrote:
> > > >> > On Wed, Sep 04, 2024 at 08:14:24AM GMT, Mike Rapoport wrote:
> > > >> >> On Tue, Sep 03, 2024 at 04:20:53PM +0200, Christian Brauner wrote:
> > > >> >> > 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(). This can be kept in place until all callers
> > > >> >> > have been ported to struct kmem_cache_args.
> > > >> >> > 
> > > >> >> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > >> >> 
> > > >> >> Reviewed-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> > > >> >> 
> > > >> >> > ---
> > > >> >> >  include/linux/slab.h | 13 ++++++++++---
> > > >> >> >  mm/slab_common.c     | 10 +++++-----
> > > >> >> >  2 files changed, 15 insertions(+), 8 deletions(-)
> > > >> >> > 
> > > >> >> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > > >> >> > index aced16a08700..4292d67094c3 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 *));
> > > >> >> 
> > > >> >> As I said earlier, this can become _kmem_cache_create and
> > > >> >> __kmem_cache_create_args can be __kmem_cache_create from the beginning.
> > > >> 
> > > >> I didn't notice an answer to this suggestion? Even if it's just that you
> > > >> don't think it's worth the rewrite, or it's not possible because X Y Z.
> > > >> Thanks.
> > > > 
> > > > I'm confused. I sent two patches as a reply to the thread plus the
> > > > answer below and there's two patches in v3 that you can use or drop.
> > > 
> > > Right, that's the part below. But the suggestion above, and also in Mike's
> > > reply to 02/12 was AFAICS to rename __kmem_cache_create_args to
> > > __kmem_cache_create (since patch 02) and here __kmem_cache_create to
> > > _kmem_cache_create. It just seemed odd to see no reaction to that (did I
> > > miss or not receive it?).
> > 
> > Oh, I see. I read it as a expressing taste and so I didn't bother
> > replying. And I really dislike single underscore function names so I
> > would like to avoid it and it also seems more confusing to me.
> 
> Heh, not quite. I don't like kmem_cache_create_args essentially becoming a
> replacement for kmem_cache_create* and I'd prefer __kmem_cache_create
> naming.
> 
> As for the single underscore, I don't have strong feelings about it, but I
> do think that it should be renamed to something else than
> __kmem_cache_create to leave __kmem_cache_create for the core function.

I honestly don't care especially because it's not visible outside of the
header. If you care then a simple patch on top of the series to rename
to whatever is fine by me.

But single vs double underscore with fundamentally different parameters
seems ripe for visual confusion and the compatibility layer will go away
anyway.