Re: [PATCH v2] landlock: Use kmem for landlock_object

[Greg KH]

On Sun, Mar 31, 2024 at 09:12:06PM +0530, Ayush Tiwari wrote:
> Hello Greg. Thanks for the feedback.
> On Sat, Mar 30, 2024 at 05:12:18PM +0100, Greg KH wrote:
> > On Sat, Mar 30, 2024 at 07:24:19PM +0530, Ayush Tiwari wrote:
> > > Use kmem_cache replace kzalloc() calls with kmem_cache_zalloc() for
> > > struct landlock_object and update the related dependencies to improve
> > > memory allocation and deallocation performance.
> > 
> > So it's faster?  Great, what are the measurements?
> > 
> Thank you for the feedback. Regarding the performance improvements, I
> realized I should have provided concrete measurements to support the
> claim. The intention behind switching to kmem_cache_zalloc() was to
> optimize memory management efficiency based on general principles.
> Could you suggest some way to get some measurements if possible?

If you can not measure the difference, why make the change at all?

Again, you need to prove the need for this change, so far I fail to see
a reason why.

> > > +static struct kmem_cache *landlock_object_cache;
> > > +
> > > +void __init landlock_object_cache_init(void)
> > > +{
> > > +	landlock_object_cache = kmem_cache_create(
> > > +		"landlock_object_cache", sizeof(struct landlock_object), 0,
> > > +		SLAB_PANIC, NULL);
> > 
> > You really want SLAB_PANIC?  Why?
> >
> The SLAB_PANIC flag used in kmem_cache_create indicates that if the
> kernel is unable to create the cache, it should panic. The use of
> SLAB_PANIC in the creation of the landlock_object_cache is due to the
> critical nature of this cache for the Landlock LSM's operation. I
> found it to be a good choice to be used. Should I use some other
> altrnative?

Is panicing really a good idea?  Why can't you properly recover from
allocation failures?

> > > +
> > >  struct landlock_object *
> > >  landlock_create_object(const struct landlock_object_underops *const underops,
> > >  		       void *const underobj)
> > > @@ -25,7 +34,8 @@ landlock_create_object(const struct landlock_object_underops *const underops,
> > >  
> > >  	if (WARN_ON_ONCE(!underops || !underobj))
> > >  		return ERR_PTR(-ENOENT);
> > > -	new_object = kzalloc(sizeof(*new_object), GFP_KERNEL_ACCOUNT);
> > > +	new_object =
> > > +		kmem_cache_zalloc(landlock_object_cache, GFP_KERNEL_ACCOUNT);
> > 
> > Odd indentation, why?
> > 
> This indentation is due to formatting introduced by running
> clang-format.

Why not keep it all on one line?

thanks,

greg k-h