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

[Linus Torvalds <torvalds@linux-foundation.org>]

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

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).

                     Linus