by Casey Schaufler

[permalink] [raw]

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

On 10/30/2014 9:03 PM, Rohit wrote:
> On Wed, 29 Oct 2014 08:12:05 -0700
> Casey Schaufler <[email protected]> wrote:
>
>> On 10/29/2014 2:11 AM, Rohit wrote:
>>> On Mon, 27 Oct 2014 09:25:28 -0700
>>> Casey Schaufler <[email protected]> wrote:
>>>
>>>> On 10/26/2014 11:54 PM, Rohit wrote:
>>>>> On Sun, 26 Oct 2014 17:41:37 -0700
>>>>> Casey Schaufler <[email protected]> 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 <[email protected]>
>>>>>>>>> To: Rohit <[email protected]>
>>>>>>>>> Cc: [email protected]; [email protected];
>>>>>>>>> [email protected]; [email protected];
>>>>>>>>> [email protected]; [email protected];
>>>>>>>>> [email protected]; [email protected];
>>>>>>>>> [email protected]; [email protected];
>>>>>>>>> [email protected]; [email protected]; Casey Schaufler
>>>>>>>>> <[email protected]> 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 <[email protected]> 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.

2014-10-31 21:32:14

by Casey Schaufler

[permalink] [raw]

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

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 <[email protected]>

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