Discussion:
[PATCH v2] Security: smack: replace kzalloc with kmem_cache for inode_smack
Rohit
2014-10-15 12:10:41 UTC
Permalink
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 <***@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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Serge E. Hallyn
2014-10-16 07:07:55 UTC
Permalink
Post by 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.
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.
Seems sensible.
Post by Rohit
---
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
Casey Schaufler
2014-10-16 16:24:01 UTC
Permalink
Post by 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.
What impact does this have on performance? I am much more
concerned with speed than with small amount of memory.
Post by Rohit
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.
---
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");
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Rohit
2014-10-17 11:42:29 UTC
Permalink
On Thu, 16 Oct 2014 09:24:01 -0700
Post by Casey Schaufler
Post by 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.
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.
Post by Casey Schaufler
Post by Rohit
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.
---
Added static in kmem_cache object declaration noted by Andrew
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Casey Schaufler
2014-10-17 14:38:53 UTC
Permalink
Post by Rohit
On Thu, 16 Oct 2014 09:24:01 -0700
Post by Casey Schaufler
Post by 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.
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.
Post by Rohit
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.
Post by Rohit
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.
Post by Rohit
Post by Casey Schaufler
Post by Rohit
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.
---
Added static in kmem_cache object declaration noted by Andrew
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Casey Schaufler
2014-10-17 17:37:21 UTC
Permalink
Hi,
________________________________
Sent: Friday, 17 October 2014 8:08 PM
Subject: Re: [PATCH v2] Security: smack: replace kzalloc with kmem_cache for inode_smack
Post by Rohit
On Thu, 16 Oct 2014 09:24:01 -0700
Post by Casey Schaufler
Post by 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.
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.
Post by Rohit
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.
Post by Rohit
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.
Post by Rohit
Post by Casey Schaufler
Post by Rohit
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.
---
Added static in kmem_cache object declaration noted by Andrew
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Loading...