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

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

[Rohit]

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.

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");
 
-- 
1.7.9.5

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

[Serge E. Hallyn]

Quoting Rohit (rohit.kr@samsung.com):
> 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.
> 
> 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>

Seems sensible.

Acked-by: Serge Hallyn <serge.hallyn@canonical.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");
>  
> -- 
> 1.7.9.5

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

[Casey Schaufler]

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.

>
> 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");
>  

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

[Rohit]

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.

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.

Please let me know your comments.
> >
> > 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

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

[Casey Schaufler]

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
>

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

[Casey Schaufler]

On 10/17/2014 9:34 AM, PINTU KUMAR wrote:
> Hi,
>
>
>> ________________________________
>> From: Casey Schaufler <casey@schaufler-ca.com>
>> To: Rohit <rohit.kr@samsung.com> 
>> Cc: akpm@linux-foundation.org; james.l.morris@oracle.com; serge@hallyn.com; linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org; cpgs@samsung.com; pintu.k@samsung.com; vishnu.ps@samsung.com; iqbal.ams@samsung.com; ed.savinay@samsung.com; me.rohit@live.com; pintu_agarwal@yahoo.com; Casey Schaufler <casey@schaufler-ca.com> 
>> Sent: Friday, 17 October 2014 8:08 PM
>> Subject: Re: [PATCH v2] Security: smack: replace kzalloc with kmem_cache for inode_smack
>>
>>
>> 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.
>>
> SMACK is used heavily in Tizen. We verified these changes for one of Tizen project.
> During boot time we observed that this object is used heavily, as identified by kmalloc-accounting.
> After replacing this we did not observe any difference in boot time. Also there was no side-effects seen so far.
> If you know of any other tests, please let us know.
> We will also try to gather some performance stats and present here.

We need to be somewhat more precise than "did not observe any
difference in boot time". The ideal benchmark would perform lots
of changes to the filesystem without doing lots of IO. One process
that matches that profile fairly well is a kernel make. I would be
satisfied with something as crude as using time(1) on a small (5?)
number of clean kernel makes each with and without the patch on the
running kernel. At the level of accuracy you usually get from time(1)
you won't find trivial differences, but if the change is a big problem
(or a big win) we'll know.
 

BTW, "Smack" is preferred to "SMACK". There's no need to shout.

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

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

[Casey Schaufler]

On 10/17/2014 10:37 AM, Casey Schaufler wrote:
> On 10/17/2014 9:34 AM, PINTU KUMAR wrote:
>> Hi,
>>
>>
>>> ________________________________
>>> From: Casey Schaufler <casey@schaufler-ca.com>
>>> To: Rohit <rohit.kr@samsung.com> 
>>> Cc: akpm@linux-foundation.org; james.l.morris@oracle.com; serge@hallyn.com; linux-security-module@vger.kernel.org; linux-kernel@vger.kernel.org; cpgs@samsung.com; pintu.k@samsung.com; vishnu.ps@samsung.com; iqbal.ams@samsung.com; ed.savinay@samsung.com; me.rohit@live.com; pintu_agarwal@yahoo.com; Casey Schaufler <casey@schaufler-ca.com> 
>>> Sent: Friday, 17 October 2014 8:08 PM
>>> Subject: Re: [PATCH v2] Security: smack: replace kzalloc with kmem_cache for inode_smack
>>>
>>>
>>> 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.
>>>
>> SMACK is used heavily in Tizen. We verified these changes for one of Tizen project.
>> During boot time we observed that this object is used heavily, as identified by kmalloc-accounting.
>> After replacing this we did not observe any difference in boot time. Also there was no side-effects seen so far.
>> If you know of any other tests, please let us know.
>> We will also try to gather some performance stats and present here.
> We need to be somewhat more precise than "did not observe any
> difference in boot time". The ideal benchmark would perform lots
> of changes to the filesystem without doing lots of IO. One process
> that matches that profile fairly well is a kernel make. I would be
> satisfied with something as crude as using time(1) on a small (5?)
> number of clean kernel makes each with and without the patch on the
> running kernel. At the level of accuracy you usually get from time(1)
> you won't find trivial differences, but if the change is a big problem
> (or a big win) we'll know.

I have not seen anything indicating that the requested performance
measurements have been done. I have no intention of accepting this
without assurance that performance has not been damaged. I request
that no one else carry this forward, either. The performance impact
of security facilities comes under too much scrutiny to ignore it.

> ... 

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

[Rohit]

On Sun, 26 Oct 2014 17:41:37 -0700
Casey Schaufler <casey@schaufler-ca.com> wrote:

> On 10/17/2014 10:37 AM, Casey Schaufler wrote:
> > On 10/17/2014 9:34 AM, PINTU KUMAR wrote:
> >> Hi,
> >>
> >>
> >>> ________________________________
> >>> From: Casey Schaufler <casey@schaufler-ca.com>
> >>> To: Rohit <rohit.kr@samsung.com> 
> >>> Cc: akpm@linux-foundation.org; james.l.morris@oracle.com;
> >>> serge@hallyn.com; linux-security-module@vger.kernel.org;
> >>> linux-kernel@vger.kernel.org; cpgs@samsung.com;
> >>> pintu.k@samsung.com; vishnu.ps@samsung.com;
> >>> iqbal.ams@samsung.com; ed.savinay@samsung.com; me.rohit@live.com;
> >>> pintu_agarwal@yahoo.com; Casey Schaufler <casey@schaufler-ca.com>
> >>> Sent: Friday, 17 October 2014 8:08 PM Subject: Re: [PATCH v2]
> >>> Security: smack: replace kzalloc with kmem_cache for inode_smack
> >>>
> >>>
> >>> 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.
> >>>
> >> SMACK is used heavily in Tizen. We verified these changes for one
> >> of Tizen project. During boot time we observed that this object is
> >> used heavily, as identified by kmalloc-accounting. After replacing
> >> this we did not observe any difference in boot time. Also there
> >> was no side-effects seen so far. If you know of any other tests,
> >> please let us know. We will also try to gather some performance
> >> stats and present here.
> > We need to be somewhat more precise than "did not observe any
> > difference in boot time". The ideal benchmark would perform lots
> > of changes to the filesystem without doing lots of IO. One process
> > that matches that profile fairly well is a kernel make. I would be
> > satisfied with something as crude as using time(1) on a small (5?)
> > number of clean kernel makes each with and without the patch on the
> > running kernel. At the level of accuracy you usually get from
> > time(1) you won't find trivial differences, but if the change is a
> > big problem (or a big win) we'll know.
> 
> I have not seen anything indicating that the requested performance
> measurements have been done. I have no intention of accepting this
> without assurance that performance has not been damaged. I request
> that no one else carry this forward, either. The performance impact
> of security facilities comes under too much scrutiny to ignore it.
> 
> > ... 
> 
Sorry for the delay as I was on holiday for last week.
Will verify the performance impact as per your suggestion.
We verified it only on Tizen based ARM board, so building kernel on it
is not possible.
I found http://elinux.org/images/0/06/Buzov-SMACK.pdf (slides - 35-37)
for performance verification of smack. It checks performance of file
creation and copy in tmpfs. 
Please let me know whether the procedure mentioned in the above
mentioned slide is fine, else please suggest some other way to check
performance on the target board.


Regards,
Rohit

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

[Casey Schaufler]

On 10/26/2014 11:54 PM, Rohit wrote:
> On Sun, 26 Oct 2014 17:41:37 -0700
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> On 10/17/2014 10:37 AM, Casey Schaufler wrote:
>>> On 10/17/2014 9:34 AM, PINTU KUMAR wrote:
>>>> Hi,
>>>>
>>>>
>>>>> ________________________________
>>>>> From: Casey Schaufler <casey@schaufler-ca.com>
>>>>> To: Rohit <rohit.kr@samsung.com> 
>>>>> Cc: akpm@linux-foundation.org; james.l.morris@oracle.com;
>>>>> serge@hallyn.com; linux-security-module@vger.kernel.org;
>>>>> linux-kernel@vger.kernel.org; cpgs@samsung.com;
>>>>> pintu.k@samsung.com; vishnu.ps@samsung.com;
>>>>> iqbal.ams@samsung.com; ed.savinay@samsung.com; me.rohit@live.com;
>>>>> pintu_agarwal@yahoo.com; Casey Schaufler <casey@schaufler-ca.com>
>>>>> Sent: Friday, 17 October 2014 8:08 PM Subject: Re: [PATCH v2]
>>>>> Security: smack: replace kzalloc with kmem_cache for inode_smack
>>>>>
>>>>>
>>>>> 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.
>>>>>
>>>> SMACK is used heavily in Tizen. We verified these changes for one
>>>> of Tizen project. During boot time we observed that this object is
>>>> used heavily, as identified by kmalloc-accounting. After replacing
>>>> this we did not observe any difference in boot time. Also there
>>>> was no side-effects seen so far. If you know of any other tests,
>>>> please let us know. We will also try to gather some performance
>>>> stats and present here.
>>> We need to be somewhat more precise than "did not observe any
>>> difference in boot time". The ideal benchmark would perform lots
>>> of changes to the filesystem without doing lots of IO. One process
>>> that matches that profile fairly well is a kernel make. I would be
>>> satisfied with something as crude as using time(1) on a small (5?)
>>> number of clean kernel makes each with and without the patch on the
>>> running kernel. At the level of accuracy you usually get from
>>> time(1) you won't find trivial differences, but if the change is a
>>> big problem (or a big win) we'll know.
>> I have not seen anything indicating that the requested performance
>> measurements have been done. I have no intention of accepting this
>> without assurance that performance has not been damaged. I request
>> that no one else carry this forward, either. The performance impact
>> of security facilities comes under too much scrutiny to ignore it.
>>
>>> ... 
> Sorry for the delay as I was on holiday for last week.
> Will verify the performance impact as per your suggestion.
> We verified it only on Tizen based ARM board, so building kernel on it
> is not possible.
> I found http://elinux.org/images/0/06/Buzov-SMACK.pdf (slides - 35-37)
> for performance verification of smack. It checks performance of file
> creation and copy in tmpfs. 
> Please let me know whether the procedure mentioned in the above
> mentioned slide is fine, else please suggest some other way to check
> performance on the target board.

The technique outlined by Buzov should provide adequate evidence.

>
>
> Regards,
> Rohit
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Rohit]

On Mon, 27 Oct 2014 09:25:28 -0700
Casey Schaufler <casey@schaufler-ca.com> wrote:

> On 10/26/2014 11:54 PM, Rohit wrote:
> > On Sun, 26 Oct 2014 17:41:37 -0700
> > Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> >> On 10/17/2014 10:37 AM, Casey Schaufler wrote:
> >>> On 10/17/2014 9:34 AM, PINTU KUMAR wrote:
> >>>> Hi,
> >>>>
> >>>>
> >>>>> ________________________________
> >>>>> From: Casey Schaufler <casey@schaufler-ca.com>
> >>>>> To: Rohit <rohit.kr@samsung.com> 
> >>>>> Cc: akpm@linux-foundation.org; james.l.morris@oracle.com;
> >>>>> serge@hallyn.com; linux-security-module@vger.kernel.org;
> >>>>> linux-kernel@vger.kernel.org; cpgs@samsung.com;
> >>>>> pintu.k@samsung.com; vishnu.ps@samsung.com;
> >>>>> iqbal.ams@samsung.com; ed.savinay@samsung.com;
> >>>>> me.rohit@live.com; pintu_agarwal@yahoo.com; Casey Schaufler
> >>>>> <casey@schaufler-ca.com> Sent: Friday, 17 October 2014 8:08 PM
> >>>>> Subject: Re: [PATCH v2] Security: smack: replace kzalloc with
> >>>>> kmem_cache for inode_smack
> >>>>>
> >>>>>
> >>>>> 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.
> >>>>>
> >>>> SMACK is used heavily in Tizen. We verified these changes for one
> >>>> of Tizen project. During boot time we observed that this object
> >>>> is used heavily, as identified by kmalloc-accounting. After
> >>>> replacing this we did not observe any difference in boot time.
> >>>> Also there was no side-effects seen so far. If you know of any
> >>>> other tests, please let us know. We will also try to gather some
> >>>> performance stats and present here.
> >>> We need to be somewhat more precise than "did not observe any
> >>> difference in boot time". The ideal benchmark would perform lots
> >>> of changes to the filesystem without doing lots of IO. One process
> >>> that matches that profile fairly well is a kernel make. I would be
> >>> satisfied with something as crude as using time(1) on a small (5?)
> >>> number of clean kernel makes each with and without the patch on
> >>> the running kernel. At the level of accuracy you usually get from
> >>> time(1) you won't find trivial differences, but if the change is a
> >>> big problem (or a big win) we'll know.
> >> I have not seen anything indicating that the requested performance
> >> measurements have been done. I have no intention of accepting this
> >> without assurance that performance has not been damaged. I request
> >> that no one else carry this forward, either. The performance impact
> >> of security facilities comes under too much scrutiny to ignore it.
> >>
> >>> ... 
> > Sorry for the delay as I was on holiday for last week.
> > Will verify the performance impact as per your suggestion.
> > We verified it only on Tizen based ARM board, so building kernel on
> > it is not possible.
> > I found http://elinux.org/images/0/06/Buzov-SMACK.pdf (slides -
> > 35-37) for performance verification of smack. It checks performance
> > of file creation and copy in tmpfs. 
> > Please let me know whether the procedure mentioned in the above
> > mentioned slide is fine, else please suggest some other way to check
> > performance on the target board.
> 
> The technique outlined by Buzov should provide adequate evidence.
We carried out file creation of 0, 1k and 4k size for 1024 files and measured the time taken.
It was done for 5 iterations for each case with kzalloc and kmem_cache on a board with 512MB RAM and 1.2 GHz dual core arm processor.
The average latency is as follows :
File size   with kzalloc(in ms)   with kmem_cache(in ms)       %change
    0           10925.6               10528.8                   -3.63
   1k           11909.8               11617.6                   -2.45
   4k           11632.2               11873.2                   +2.07

>From the data, it seems that is no significant difference in performance.
Please let me know your opinion.
> 
> >
> >
> > Regards,
> > Rohit
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-security-module" in the body of a message to
> > majordomo@vger.kernel.org More majordomo info at
> > http://vger.kernel.org/majordomo-info.html
> >
> 

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

[Casey Schaufler]

On 10/29/2014 2:11 AM, Rohit wrote:
> On Mon, 27 Oct 2014 09:25:28 -0700
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> On 10/26/2014 11:54 PM, Rohit wrote:
>>> On Sun, 26 Oct 2014 17:41:37 -0700
>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>
>>>> On 10/17/2014 10:37 AM, Casey Schaufler wrote:
>>>>> On 10/17/2014 9:34 AM, PINTU KUMAR wrote:
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>>> ________________________________
>>>>>>> From: Casey Schaufler <casey@schaufler-ca.com>
>>>>>>> To: Rohit <rohit.kr@samsung.com> 
>>>>>>> Cc: akpm@linux-foundation.org; james.l.morris@oracle.com;
>>>>>>> serge@hallyn.com; linux-security-module@vger.kernel.org;
>>>>>>> linux-kernel@vger.kernel.org; cpgs@samsung.com;
>>>>>>> pintu.k@samsung.com; vishnu.ps@samsung.com;
>>>>>>> iqbal.ams@samsung.com; ed.savinay@samsung.com;
>>>>>>> me.rohit@live.com; pintu_agarwal@yahoo.com; Casey Schaufler
>>>>>>> <casey@schaufler-ca.com> Sent: Friday, 17 October 2014 8:08 PM
>>>>>>> Subject: Re: [PATCH v2] Security: smack: replace kzalloc with
>>>>>>> kmem_cache for inode_smack
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>> SMACK is used heavily in Tizen. We verified these changes for one
>>>>>> of Tizen project. During boot time we observed that this object
>>>>>> is used heavily, as identified by kmalloc-accounting. After
>>>>>> replacing this we did not observe any difference in boot time.
>>>>>> Also there was no side-effects seen so far. If you know of any
>>>>>> other tests, please let us know. We will also try to gather some
>>>>>> performance stats and present here.
>>>>> We need to be somewhat more precise than "did not observe any
>>>>> difference in boot time". The ideal benchmark would perform lots
>>>>> of changes to the filesystem without doing lots of IO. One process
>>>>> that matches that profile fairly well is a kernel make. I would be
>>>>> satisfied with something as crude as using time(1) on a small (5?)
>>>>> number of clean kernel makes each with and without the patch on
>>>>> the running kernel. At the level of accuracy you usually get from
>>>>> time(1) you won't find trivial differences, but if the change is a
>>>>> big problem (or a big win) we'll know.
>>>> I have not seen anything indicating that the requested performance
>>>> measurements have been done. I have no intention of accepting this
>>>> without assurance that performance has not been damaged. I request
>>>> that no one else carry this forward, either. The performance impact
>>>> of security facilities comes under too much scrutiny to ignore it.
>>>>
>>>>> ... 
>>> Sorry for the delay as I was on holiday for last week.
>>> Will verify the performance impact as per your suggestion.
>>> We verified it only on Tizen based ARM board, so building kernel on
>>> it is not possible.
>>> I found http://elinux.org/images/0/06/Buzov-SMACK.pdf (slides -
>>> 35-37) for performance verification of smack. It checks performance
>>> of file creation and copy in tmpfs. 
>>> Please let me know whether the procedure mentioned in the above
>>> mentioned slide is fine, else please suggest some other way to check
>>> performance on the target board.
>> The technique outlined by Buzov should provide adequate evidence.
> We carried out file creation of 0, 1k and 4k size for 1024 files and measured the time taken.
> It was done for 5 iterations for each case with kzalloc and kmem_cache on a board with 512MB RAM and 1.2 GHz dual core arm processor.
> The average latency is as follows :
> File size   with kzalloc(in ms)   with kmem_cache(in ms)       %change
>     0           10925.6               10528.8                   -3.63
>    1k           11909.8               11617.6                   -2.45
>    4k           11632.2               11873.2                   +2.07
>
> From the data, it seems that is no significant difference in performance.
> Please let me know your opinion.

The data is kind of scary, don't you think? The performance
starts out as an improvement, but gets worse as file size gets
bigger. This could be anomalous with your choice of file size.
We see that kzalloc performance improves going from 1k to 4k.

Can you run the same test with 10 (20 would be better) iterations
for 0, 1k, 3k, 4k, 5k, 20k, 100k and 1m?

I am curious now. I will run my own tests as well.

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

[Rohit]

On Wed, 29 Oct 2014 08:12:05 -0700
Casey Schaufler <casey@schaufler-ca.com> wrote:

> On 10/29/2014 2:11 AM, Rohit wrote:
> > On Mon, 27 Oct 2014 09:25:28 -0700
> > Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> >> On 10/26/2014 11:54 PM, Rohit wrote:
> >>> On Sun, 26 Oct 2014 17:41:37 -0700
> >>> Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>
> >>>> On 10/17/2014 10:37 AM, Casey Schaufler wrote:
> >>>>> On 10/17/2014 9:34 AM, PINTU KUMAR wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>>
> >>>>>>> ________________________________
> >>>>>>> From: Casey Schaufler <casey@schaufler-ca.com>
> >>>>>>> To: Rohit <rohit.kr@samsung.com> 
> >>>>>>> Cc: akpm@linux-foundation.org; james.l.morris@oracle.com;
> >>>>>>> serge@hallyn.com; linux-security-module@vger.kernel.org;
> >>>>>>> linux-kernel@vger.kernel.org; cpgs@samsung.com;
> >>>>>>> pintu.k@samsung.com; vishnu.ps@samsung.com;
> >>>>>>> iqbal.ams@samsung.com; ed.savinay@samsung.com;
> >>>>>>> me.rohit@live.com; pintu_agarwal@yahoo.com; Casey Schaufler
> >>>>>>> <casey@schaufler-ca.com> Sent: Friday, 17 October 2014 8:08 PM
> >>>>>>> Subject: Re: [PATCH v2] Security: smack: replace kzalloc with
> >>>>>>> kmem_cache for inode_smack
> >>>>>>>
> >>>>>>>
> >>>>>>> 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.
> >>>>>>>
> >>>>>> SMACK is used heavily in Tizen. We verified these changes for
> >>>>>> one of Tizen project. During boot time we observed that this
> >>>>>> object is used heavily, as identified by kmalloc-accounting.
> >>>>>> After replacing this we did not observe any difference in boot
> >>>>>> time. Also there was no side-effects seen so far. If you know
> >>>>>> of any other tests, please let us know. We will also try to
> >>>>>> gather some performance stats and present here.
> >>>>> We need to be somewhat more precise than "did not observe any
> >>>>> difference in boot time". The ideal benchmark would perform lots
> >>>>> of changes to the filesystem without doing lots of IO. One
> >>>>> process that matches that profile fairly well is a kernel make.
> >>>>> I would be satisfied with something as crude as using time(1)
> >>>>> on a small (5?) number of clean kernel makes each with and
> >>>>> without the patch on the running kernel. At the level of
> >>>>> accuracy you usually get from time(1) you won't find trivial
> >>>>> differences, but if the change is a big problem (or a big win)
> >>>>> we'll know.
> >>>> I have not seen anything indicating that the requested
> >>>> performance measurements have been done. I have no intention of
> >>>> accepting this without assurance that performance has not been
> >>>> damaged. I request that no one else carry this forward, either.
> >>>> The performance impact of security facilities comes under too
> >>>> much scrutiny to ignore it.
> >>>>
> >>>>> ... 
> >>> Sorry for the delay as I was on holiday for last week.
> >>> Will verify the performance impact as per your suggestion.
> >>> We verified it only on Tizen based ARM board, so building kernel
> >>> on it is not possible.
> >>> I found http://elinux.org/images/0/06/Buzov-SMACK.pdf (slides -
> >>> 35-37) for performance verification of smack. It checks
> >>> performance of file creation and copy in tmpfs. 
> >>> Please let me know whether the procedure mentioned in the above
> >>> mentioned slide is fine, else please suggest some other way to
> >>> check performance on the target board.
> >> The technique outlined by Buzov should provide adequate evidence.
> > We carried out file creation of 0, 1k and 4k size for 1024 files
> > and measured the time taken. It was done for 5 iterations for each
> > case with kzalloc and kmem_cache on a board with 512MB RAM and 1.2
> > GHz dual core arm processor. The average latency is as follows :
> > File size   with kzalloc(in ms)   with kmem_cache(in ms)
> > %change 0           10925.6               10528.8
> > -3.63 1k           11909.8               11617.6
> > -2.45 4k           11632.2               11873.2
> > +2.07
> >
> > From the data, it seems that is no significant difference in
> > performance. Please let me know your opinion.
> 
> The data is kind of scary, don't you think? The performance
> starts out as an improvement, but gets worse as file size gets
> bigger. This could be anomalous with your choice of file size.
> We see that kzalloc performance improves going from 1k to 4k.
> 
> Can you run the same test with 10 (20 would be better) iterations
> for 0, 1k, 3k, 4k, 5k, 20k, 100k and 1m?
> 
> I am curious now. I will run my own tests as well.
>

Yes, i too found it odd for 4k file taking less time than 1k size.
Actually, issue was probably because I was using different block size
for file creation in two cases.

The latency for creating 1024 files with average of  10 iterations for
below file size is as follows:
File size   With Kzalloc(in ms)  With kmem_cache(in ms)     %change
0               10610.6              10595.4                 -0.14 
1k              11832.3              11667.4                 -1.39 
4k              11861.2              11802.1                 -0.49 
20k             11991.7              11977.7                 -0.11 
50k             12242.9              12224.7                 -0.14 
100k            12638.9              12581                   -0.45

It seems kmem_cache has little better performance compared to kzalloc.
Please share your comments and your test results,if any.


 

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

[Casey Schaufler]

On 10/30/2014 9:03 PM, Rohit wrote:
> On Wed, 29 Oct 2014 08:12:05 -0700
> Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>> On 10/29/2014 2:11 AM, Rohit wrote:
>>> On Mon, 27 Oct 2014 09:25:28 -0700
>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>
>>>> On 10/26/2014 11:54 PM, Rohit wrote:
>>>>> On Sun, 26 Oct 2014 17:41:37 -0700
>>>>> Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>
>>>>>> On 10/17/2014 10:37 AM, Casey Schaufler wrote:
>>>>>>> On 10/17/2014 9:34 AM, PINTU KUMAR wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>>
>>>>>>>>> ________________________________
>>>>>>>>> From: Casey Schaufler <casey@schaufler-ca.com>
>>>>>>>>> To: Rohit <rohit.kr@samsung.com> 
>>>>>>>>> Cc: akpm@linux-foundation.org; james.l.morris@oracle.com;
>>>>>>>>> serge@hallyn.com; linux-security-module@vger.kernel.org;
>>>>>>>>> linux-kernel@vger.kernel.org; cpgs@samsung.com;
>>>>>>>>> pintu.k@samsung.com; vishnu.ps@samsung.com;
>>>>>>>>> iqbal.ams@samsung.com; ed.savinay@samsung.com;
>>>>>>>>> me.rohit@live.com; pintu_agarwal@yahoo.com; Casey Schaufler
>>>>>>>>> <casey@schaufler-ca.com> Sent: Friday, 17 October 2014 8:08 PM
>>>>>>>>> Subject: Re: [PATCH v2] Security: smack: replace kzalloc with
>>>>>>>>> kmem_cache for inode_smack
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>> SMACK is used heavily in Tizen. We verified these changes for
>>>>>>>> one of Tizen project. During boot time we observed that this
>>>>>>>> object is used heavily, as identified by kmalloc-accounting.
>>>>>>>> After replacing this we did not observe any difference in boot
>>>>>>>> time. Also there was no side-effects seen so far. If you know
>>>>>>>> of any other tests, please let us know. We will also try to
>>>>>>>> gather some performance stats and present here.
>>>>>>> We need to be somewhat more precise than "did not observe any
>>>>>>> difference in boot time". The ideal benchmark would perform lots
>>>>>>> of changes to the filesystem without doing lots of IO. One
>>>>>>> process that matches that profile fairly well is a kernel make.
>>>>>>> I would be satisfied with something as crude as using time(1)
>>>>>>> on a small (5?) number of clean kernel makes each with and
>>>>>>> without the patch on the running kernel. At the level of
>>>>>>> accuracy you usually get from time(1) you won't find trivial
>>>>>>> differences, but if the change is a big problem (or a big win)
>>>>>>> we'll know.
>>>>>> I have not seen anything indicating that the requested
>>>>>> performance measurements have been done. I have no intention of
>>>>>> accepting this without assurance that performance has not been
>>>>>> damaged. I request that no one else carry this forward, either.
>>>>>> The performance impact of security facilities comes under too
>>>>>> much scrutiny to ignore it.
>>>>>>
>>>>>>> ... 
>>>>> Sorry for the delay as I was on holiday for last week.
>>>>> Will verify the performance impact as per your suggestion.
>>>>> We verified it only on Tizen based ARM board, so building kernel
>>>>> on it is not possible.
>>>>> I found http://elinux.org/images/0/06/Buzov-SMACK.pdf (slides -
>>>>> 35-37) for performance verification of smack. It checks
>>>>> performance of file creation and copy in tmpfs. 
>>>>> Please let me know whether the procedure mentioned in the above
>>>>> mentioned slide is fine, else please suggest some other way to
>>>>> check performance on the target board.
>>>> The technique outlined by Buzov should provide adequate evidence.
>>> We carried out file creation of 0, 1k and 4k size for 1024 files
>>> and measured the time taken. It was done for 5 iterations for each
>>> case with kzalloc and kmem_cache on a board with 512MB RAM and 1.2
>>> GHz dual core arm processor. The average latency is as follows :
>>> File size   with kzalloc(in ms)   with kmem_cache(in ms)
>>> %change 0           10925.6               10528.8
>>> -3.63 1k           11909.8               11617.6
>>> -2.45 4k           11632.2               11873.2
>>> +2.07
>>>
>>> From the data, it seems that is no significant difference in
>>> performance. Please let me know your opinion.
>> The data is kind of scary, don't you think? The performance
>> starts out as an improvement, but gets worse as file size gets
>> bigger. This could be anomalous with your choice of file size.
>> We see that kzalloc performance improves going from 1k to 4k.
>>
>> Can you run the same test with 10 (20 would be better) iterations
>> for 0, 1k, 3k, 4k, 5k, 20k, 100k and 1m?
>>
>> I am curious now. I will run my own tests as well.
>>
> Yes, i too found it odd for 4k file taking less time than 1k size.
> Actually, issue was probably because I was using different block size
> for file creation in two cases.
>
> The latency for creating 1024 files with average of  10 iterations for
> below file size is as follows:
> File size   With Kzalloc(in ms)  With kmem_cache(in ms)     %change
> 0               10610.6              10595.4                 -0.14 
> 1k              11832.3              11667.4                 -1.39 
> 4k              11861.2              11802.1                 -0.49 
> 20k             11991.7              11977.7                 -0.11 
> 50k             12242.9              12224.7                 -0.14 
> 100k            12638.9              12581                   -0.45
>
> It seems kmem_cache has little better performance compared to kzalloc.
> Please share your comments and your test results,if any.

I got similar results with the kernel make tests. I will accept the patch.
Look for a message to that affect later today.

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

[Casey Schaufler]

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

Applied to git://git.gitorious.org/smack-next/kernel.git#smack-for-3.19


> ---
> 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");
>