Messages in this thread Patch in this message | | | Date | Wed, 3 Apr 2024 18:09:27 +0200 | From | Mickaël Salaün <> | Subject | Re: [PATCH v2] landlock: Use kmem for landlock_object |
| |
On Sun, Mar 31, 2024 at 11:38:28PM +0530, Ayush Tiwari wrote: > On Sun, Mar 31, 2024 at 06:54:39PM +0200, Greg KH wrote: > > 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? > > Kindly refer to this issue: https://github.com/landlock-lsm/linux/issues/19 > I have been assigned this issue hence I am focussing on making the > changes that have been listed.
As Greg asked, it would be good know the performance impact of such change. This could be measured by creating a lot of related allocations and accessing them in non-sequential order (e.g. adding new rules, accessing a related inode while being sandboxed). I guess there will be a lot of noise (because of other subsystems) but it's worth a try. You should look at similar commits and their related threads to see what others did.
> > > > 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? > > I am relying on SLAB_PANIC because of the reason I mentioned earlier, > and also because it was used in lsm_file_cache that I was asked to look > into as reference. I could try to recover from allocation failures but > currently my focus is on working on the changes that are listed. I will > definitely try to look into it once I am done with all changes.
Not being able to create this kmem cache would mean that Landlock would not be able to properly run, so we could print a warning and exit the Landlock init function. However, most calls to kmem_cache_create() are init calls, and most of them (especially in security/*) set SLAB_PANIC. I'm wondering why Landlock should do differently, if others should be fixed, and if the extra complexity of handling several kmem_cache_create() potential failure is worth it for init handlers?
> > > > > > + > > > > > 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? > > > I kept it all in one line in v1, but Paul and Mickael asked me to use > clang-format, hence it is this way.
Yes, it may looks weird but we format everything with clang-format to not waste time discussing about style.
> > thanks, > > > > greg k-h >
| |