Re: [PATCH v2] Security: smack: replace kzalloc with kmem_cache for inode_smack

[Casey Schaufler <casey@schaufler-ca.com>]

On 10/17/2014 4:42 AM, Rohit wrote:
> On Thu, 16 Oct 2014 09:24:01 -0700
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> On 10/15/2014 5:10 AM, Rohit wrote:
>>> The patch use kmem_cache to allocate/free inode_smack since they are
>>> alloced in high volumes making it a perfect case for kmem_cache.
>>>
>>> As per analysis, 24 bytes of memory is wasted per allocation due
>>> to internal fragmentation. With kmem_cache, this can be avoided.
>> What impact does this have on performance? I am much more
>> concerned with speed than with small amount of memory.
>>
> I think there should not be any performance problem as such.
> However, please let me know how to check the performance in this case.

Any inode intensive benchmark would suffice. Even the classic
kernel build would do.

> As far as i know, kzalloc first finds the kmalloc_index corresponding to
> the size to get the kmem_cache_object and then calls kmem_cache_alloc
> with the kmalloc_index(kmem_cache object). Here, we create kmem_cache
> object specific for inode_smack and directly calls kmem_cache_alloc()
> which should give better performance as compared to kzalloc.

That would be my guess as well, but performance is tricky. Sometimes
things that "obviously" make performance better make it worse. There can
be unanticipated side effects.


> Please let me know your comments.

If you can run any sort of test that demonstrates this change
does not have performance impact, I'm fine with it. Smack is being
used in small devices, and both memory use and performance are critical
to the success of these devices. Of the two, performance is currently
more of an issue.

Thank you.

>>> Accounting of memory allocation is below :
>>>  total       slack            net      count-alloc/free
>>> caller Before (with kzalloc)
>>> 1919872      719952          1919872      29998/0
>>> new_inode_smack+0x14
>>>
>>> After (with kmem_cache)
>>> 1201680          0           1201680      30042/0
>>> new_inode_smack+0x18
>>>
>>> >From above data, we found that 719952 bytes(~700 KB) of memory is
>>> saved on allocation of 29998 smack inodes.
>>>
>>> Signed-off-by: Rohit <rohit.kr@samsung.com>
>>> ---
>>> Added static in kmem_cache object declaration noted by Andrew
>>> Morton <akpm@ linux-foundation.org> . Also updated commit message.
>>>  security/smack/smack_lsm.c |   13 ++++++++++---
>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>> index d515ec2..15d985c 100644
>>> --- a/security/smack/smack_lsm.c
>>> +++ b/security/smack/smack_lsm.c
>>> @@ -53,6 +53,7 @@
>>>  #define SMK_SENDING	2
>>>  
>>>  LIST_HEAD(smk_ipv6_port_list);
>>> +static struct kmem_cache *smack_inode_cache;
>>>  
>>>  #ifdef CONFIG_SECURITY_SMACK_BRINGUP
>>>  static void smk_bu_mode(int mode, char *s)
>>> @@ -240,7 +241,7 @@ struct inode_smack *new_inode_smack(struct
>>> smack_known *skp) {
>>>  	struct inode_smack *isp;
>>>  
>>> -	isp = kzalloc(sizeof(struct inode_smack), GFP_NOFS);
>>> +	isp = kmem_cache_zalloc(smack_inode_cache, GFP_NOFS);
>>>  	if (isp == NULL)
>>>  		return NULL;
>>>  
>>> @@ -767,7 +768,7 @@ static int smack_inode_alloc_security(struct
>>> inode *inode) */
>>>  static void smack_inode_free_security(struct inode *inode)
>>>  {
>>> -	kfree(inode->i_security);
>>> +	kmem_cache_free(smack_inode_cache, inode->i_security);
>>>  	inode->i_security = NULL;
>>>  }
>>>  
>>> @@ -4264,10 +4265,16 @@ static __init int smack_init(void)
>>>  	if (!security_module_enable(&smack_ops))
>>>  		return 0;
>>>  
>>> +	smack_inode_cache = KMEM_CACHE(inode_smack, 0);
>>> +	if (!smack_inode_cache)
>>> +		return -ENOMEM;
>>> +
>>>  	tsp = new_task_smack(&smack_known_floor,
>>> &smack_known_floor, GFP_KERNEL);
>>> -	if (tsp == NULL)
>>> +	if (tsp == NULL) {
>>> +		kmem_cache_destroy(smack_inode_cache);
>>>  		return -ENOMEM;
>>> +	}
>>>  
>>>  	printk(KERN_INFO "Smack:  Initializing.\n");
>>>  
> Thanks,
> Rohit
>