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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 04, 2024 at 11:53:05AM GMT, Linus Torvalds wrote:
> On Wed, 4 Sept 2024 at 11:21, Christian Brauner <brauner@xxxxxxxxxx> 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.





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux