[PATCH 0/5] kstrdup optimization

[PATCH 0/5] kstrdup optimization

[Andrzej Hajda]

Hi,

kstrdup if often used to duplicate strings where neither source neither
destination will be ever modified. In such case we can just reuse the source
instead of duplicating it. The problem is that we must be sure that
the source is non-modifiable and its life-time is long enough.

I suspect the good candidates for such strings are strings located in kernel
.rodata section, they cannot be modifed because the section is read-only and
their life-time is equal to kernel life-time.

This small patchset proposes alternative version of kstrdup - kstrdup_const,
which returns source string if it is located in .rodata otherwise it fallbacks
to kstrdup.
To verify if the source is in .rodata function checks if the address is between
sentinels __start_rodata, __end_rodata. I guess it should work with all
architectures.

The main patch is accompanied by four patches constifying kstrdup for cases
where situtation described above happens frequently.

As I have tested the patchset on mobile platform (exynos4210-trats) it saves
3272 string allocations. Since minimal allocation is 32 or 64 bytes depending
on Kconfig options the patchset saves respectively about 100KB or 200KB of memory.

The patchset is based on 3.19-rc4.

This patchset have been already sent to the list as RFC.
Current version have following changes:
- added missing export,
- added kerneldocs,
- constified kstrdup in VFS devname allocation.

Regards
Andrzej


Andrzej Hajda (5):
  mm/util: add kstrdup_const
  kernfs: convert node name allocation to kstrdup_const
  clk: convert clock name allocations to kstrdup_const
  mm/slab: convert cache name allocations to kstrdup_const
  fs/namespace: convert devname allocation to kstrdup_const

 drivers/clk/clk.c      | 12 ++++++------
 fs/kernfs/dir.c        | 12 ++++++------
 fs/namespace.c         |  6 +++---
 include/linux/string.h |  3 +++
 mm/slab_common.c       |  6 +++---
 mm/util.c              | 38 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 59 insertions(+), 18 deletions(-)

-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/5] mm/util: add kstrdup_const

[Andrzej Hajda]

The patch adds alternative version of kstrdup which returns pointer
to constant char array. The function checks if input string is in
persistent and read-only memory section, if yes it returns the input string,
otherwise it fallbacks to kstrdup.
kstrdup_const is accompanied by kfree_const performing conditional memory
deallocation of the string.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 include/linux/string.h |  3 +++
 mm/util.c              | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 2e22a2e..b11ed1e 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -115,7 +115,10 @@ extern void * memchr(const void *,int,__kernel_size_t);
 #endif
 void *memchr_inv(const void *s, int c, size_t n);
 
+extern void kfree_const(const void *x);
+
 extern char *kstrdup(const char *s, gfp_t gfp);
+extern const char *kstrdup_const(const char *s, gfp_t gfp);
 extern char *kstrndup(const char *s, size_t len, gfp_t gfp);
 extern void *kmemdup(const void *src, size_t len, gfp_t gfp);
 
diff --git a/mm/util.c b/mm/util.c
index fec39d4..7c62128 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -12,10 +12,30 @@
 #include <linux/hugetlb.h>
 #include <linux/vmalloc.h>
 
+#include <asm/sections.h>
 #include <asm/uaccess.h>
 
 #include "internal.h"
 
+static inline int is_kernel_rodata(unsigned long addr)
+{
+	return addr >= (unsigned long)__start_rodata &&
+		addr < (unsigned long)__end_rodata;
+}
+
+/**
+ * kfree_const - conditionally free memory
+ * @x: pointer to the memory
+ *
+ * Function calls kfree only if @x is not in .rodata section.
+ */
+void kfree_const(const void *x)
+{
+	if (!is_kernel_rodata((unsigned long)x))
+		kfree(x);
+}
+EXPORT_SYMBOL(kfree_const);
+
 /**
  * kstrdup - allocate space for and copy an existing string
  * @s: the string to duplicate
@@ -38,6 +58,24 @@ char *kstrdup(const char *s, gfp_t gfp)
 EXPORT_SYMBOL(kstrdup);
 
 /**
+ * kstrdup_const - conditionally duplicate an existing const string
+ * @s: the string to duplicate
+ * @gfp: the GFP mask used in the kmalloc() call when allocating memory
+ *
+ * Function returns source string if it is in .rodata section otherwise it
+ * fallbacks to kstrdup.
+ * Strings allocated by kstrdup_const should be freed by kfree_const.
+ */
+const char *kstrdup_const(const char *s, gfp_t gfp)
+{
+	if (is_kernel_rodata((unsigned long)s))
+		return s;
+
+	return kstrdup(s, gfp);
+}
+EXPORT_SYMBOL(kstrdup_const);
+
+/**
  * kstrndup - allocate space for and copy an existing string
  * @s: the string to duplicate
  * @max: read at most @max chars from @s
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/5] mm/util: add kstrdup_const

[Joe Perches]

On Mon, 2015-01-12 at 10:18 +0100, Andrzej Hajda wrote:
> The patch adds alternative version of kstrdup which returns pointer
> to constant char array. The function checks if input string is in
> persistent and read-only memory section, if yes it returns the input string,
> otherwise it fallbacks to kstrdup.
> kstrdup_const is accompanied by kfree_const performing conditional memory
> deallocation of the string.

trivia:

> diff --git a/mm/util.c b/mm/util.c
[]
> +void kfree_const(const void *x)
> +{
> +	if (!is_kernel_rodata((unsigned long)x))
> +		kfree(x);
> +}
> +EXPORT_SYMBOL(kfree_const);
[]
> +const char *kstrdup_const(const char *s, gfp_t gfp)
> +{
> +	if (is_kernel_rodata((unsigned long)s))
> +		return s;
> +
> +	return kstrdup(s, gfp);
> +}
> +EXPORT_SYMBOL(kstrdup_const);

I think it'd be nicer if these used the same form
even if it's a vertical line or 2 longer

void kfree_const(const void *x)
{
	if (is_kernel_rodata((unsigned long)x))
		return;

	kfree(x);
}


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/5] kernfs: convert node name allocation to kstrdup_const

[Andrzej Hajda]

sysfs frequently performs duplication of strings located
in read-only memory section. Replacing kstrdup by kstrdup_const
allows to avoid such operations.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 fs/kernfs/dir.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 37989f0..259e92a 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -408,7 +408,7 @@ void kernfs_put(struct kernfs_node *kn)
 	if (kernfs_type(kn) == KERNFS_LINK)
 		kernfs_put(kn->symlink.target_kn);
 	if (!(kn->flags & KERNFS_STATIC_NAME))
-		kfree(kn->name);
+		kfree_const(kn->name);
 	if (kn->iattr) {
 		if (kn->iattr->ia_secdata)
 			security_release_secctx(kn->iattr->ia_secdata,
@@ -502,12 +502,12 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
 					     const char *name, umode_t mode,
 					     unsigned flags)
 {
-	char *dup_name = NULL;
+	const char *dup_name = NULL;
 	struct kernfs_node *kn;
 	int ret;
 
 	if (!(flags & KERNFS_STATIC_NAME)) {
-		name = dup_name = kstrdup(name, GFP_KERNEL);
+		name = dup_name = kstrdup_const(name, GFP_KERNEL);
 		if (!name)
 			return NULL;
 	}
@@ -534,7 +534,7 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
  err_out2:
 	kmem_cache_free(kernfs_node_cache, kn);
  err_out1:
-	kfree(dup_name);
+	kfree_const(dup_name);
 	return NULL;
 }
 
@@ -1260,7 +1260,7 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 	/* rename kernfs_node */
 	if (strcmp(kn->name, new_name) != 0) {
 		error = -ENOMEM;
-		new_name = kstrdup(new_name, GFP_KERNEL);
+		new_name = kstrdup_const(new_name, GFP_KERNEL);
 		if (!new_name)
 			goto out;
 	} else {
@@ -1293,7 +1293,7 @@ int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
 	kernfs_link_sibling(kn);
 
 	kernfs_put(old_parent);
-	kfree(old_name);
+	kfree_const(old_name);
 
 	error = 0;
  out:
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/5] kernfs: convert node name allocation to kstrdup_const

[Tejun Heo]

On Mon, Jan 12, 2015 at 10:18:40AM +0100, Andrzej Hajda wrote:
> sysfs frequently performs duplication of strings located
> in read-only memory section. Replacing kstrdup by kstrdup_const
> allows to avoid such operations.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2.5/5] kernfs: remove KERNFS_STATIC_NAME

[Tejun Heo]

When a new kernfs node is created, KERNFS_STATIC_NAME is used to avoid
making a separate copy of its name.  It's currently only used for
sysfs attributes whose filenames are required to stay accessible and
unchanged.  There are rare exceptions where these names are allocated
and formatted dynamically but for the vast majority of cases they're
consts in the rodata section.

Now that kernfs is converted to use kstrdup_const() and kfree_const(),
there's little point in keeping KERNFS_STATIC_NAME around.  Remove it.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
Lightly tested.  Seems to work fine.  Please feel free to route with
the rest of the series.

Thanks.

 fs/kernfs/dir.c        |   20 ++++++++------------
 fs/kernfs/file.c       |    4 ----
 fs/sysfs/file.c        |    2 +-
 include/linux/kernfs.h |    7 ++-----
 kernel/cgroup.c        |    2 +-
 5 files changed, 12 insertions(+), 23 deletions(-)

--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -407,8 +407,9 @@ void kernfs_put(struct kernfs_node *kn)
 
 	if (kernfs_type(kn) == KERNFS_LINK)
 		kernfs_put(kn->symlink.target_kn);
-	if (!(kn->flags & KERNFS_STATIC_NAME))
-		kfree_const(kn->name);
+
+	kfree_const(kn->name);
+
 	if (kn->iattr) {
 		if (kn->iattr->ia_secdata)
 			security_release_secctx(kn->iattr->ia_secdata,
@@ -502,15 +503,12 @@ static struct kernfs_node *__kernfs_new_
 					     const char *name, umode_t mode,
 					     unsigned flags)
 {
-	const char *dup_name = NULL;
 	struct kernfs_node *kn;
 	int ret;
 
-	if (!(flags & KERNFS_STATIC_NAME)) {
-		name = dup_name = kstrdup_const(name, GFP_KERNEL);
-		if (!name)
-			return NULL;
-	}
+	name = kstrdup_const(name, GFP_KERNEL);
+	if (!name)
+		return NULL;
 
 	kn = kmem_cache_zalloc(kernfs_node_cache, GFP_KERNEL);
 	if (!kn)
@@ -534,7 +532,7 @@ static struct kernfs_node *__kernfs_new_
  err_out2:
 	kmem_cache_free(kernfs_node_cache, kn);
  err_out1:
-	kfree_const(dup_name);
+	kfree_const(name);
 	return NULL;
 }
 
@@ -1281,9 +1279,7 @@ int kernfs_rename_ns(struct kernfs_node
 
 	kn->ns = new_ns;
 	if (new_name) {
-		if (!(kn->flags & KERNFS_STATIC_NAME))
-			old_name = kn->name;
-		kn->flags &= ~KERNFS_STATIC_NAME;
+		old_name = kn->name;
 		kn->name = new_name;
 	}
 
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -901,7 +901,6 @@ const struct file_operations kernfs_file
  * @ops: kernfs operations for the file
  * @priv: private data for the file
  * @ns: optional namespace tag of the file
- * @name_is_static: don't copy file name
  * @key: lockdep key for the file's active_ref, %NULL to disable lockdep
  *
  * Returns the created node on success, ERR_PTR() value on error.
@@ -911,7 +910,6 @@ struct kernfs_node *__kernfs_create_file
 					 umode_t mode, loff_t size,
 					 const struct kernfs_ops *ops,
 					 void *priv, const void *ns,
-					 bool name_is_static,
 					 struct lock_class_key *key)
 {
 	struct kernfs_node *kn;
@@ -919,8 +917,6 @@ struct kernfs_node *__kernfs_create_file
 	int rc;
 
 	flags = KERNFS_FILE;
-	if (name_is_static)
-		flags |= KERNFS_STATIC_NAME;
 
 	kn = kernfs_new_node(parent, name, (mode & S_IALLUGO) | S_IFREG, flags);
 	if (!kn)
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -295,7 +295,7 @@ int sysfs_add_file_mode_ns(struct kernfs
 		key = attr->key ?: (struct lock_class_key *)&attr->skey;
 #endif
 	kn = __kernfs_create_file(parent, attr->name, mode & 0777, size, ops,
-				  (void *)attr, ns, true, key);
+				  (void *)attr, ns, key);
 	if (IS_ERR(kn)) {
 		if (PTR_ERR(kn) == -EEXIST)
 			sysfs_warn_dup(parent, attr->name);
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -43,7 +43,6 @@ enum kernfs_node_flag {
 	KERNFS_HAS_SEQ_SHOW	= 0x0040,
 	KERNFS_HAS_MMAP		= 0x0080,
 	KERNFS_LOCKDEP		= 0x0100,
-	KERNFS_STATIC_NAME	= 0x0200,
 	KERNFS_SUICIDAL		= 0x0400,
 	KERNFS_SUICIDED		= 0x0800,
 };
@@ -291,7 +290,6 @@ struct kernfs_node *__kernfs_create_file
 					 umode_t mode, loff_t size,
 					 const struct kernfs_ops *ops,
 					 void *priv, const void *ns,
-					 bool name_is_static,
 					 struct lock_class_key *key);
 struct kernfs_node *kernfs_create_link(struct kernfs_node *parent,
 				       const char *name,
@@ -369,8 +367,7 @@ kernfs_create_dir_ns(struct kernfs_node
 static inline struct kernfs_node *
 __kernfs_create_file(struct kernfs_node *parent, const char *name,
 		     umode_t mode, loff_t size, const struct kernfs_ops *ops,
-		     void *priv, const void *ns, bool name_is_static,
-		     struct lock_class_key *key)
+		     void *priv, const void *ns, struct lock_class_key *key)
 { return ERR_PTR(-ENOSYS); }
 
 static inline struct kernfs_node *
@@ -439,7 +436,7 @@ kernfs_create_file_ns(struct kernfs_node
 	key = (struct lock_class_key *)&ops->lockdep_key;
 #endif
 	return __kernfs_create_file(parent, name, mode, size, ops, priv, ns,
-				    false, key);
+				    key);
 }
 
 static inline struct kernfs_node *
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -3077,7 +3077,7 @@ static int cgroup_add_file(struct cgroup
 #endif
 	kn = __kernfs_create_file(cgrp->kn, cgroup_file_name(cgrp, cft, name),
 				  cgroup_file_mode(cft), 0, cft->kf_ops, cft,
-				  NULL, false, key);
+				  NULL, key);
 	if (IS_ERR(kn))
 		return PTR_ERR(kn);
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/5] clk: convert clock name allocations to kstrdup_const

[Andrzej Hajda]

Clock subsystem frequently performs duplication of strings located
in read-only memory section. Replacing kstrdup by kstrdup_const
allows to avoid such operations.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/clk/clk.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f4963b7..27e644a 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2048,7 +2048,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		goto fail_out;
 	}
 
-	clk->name = kstrdup(hw->init->name, GFP_KERNEL);
+	clk->name = kstrdup_const(hw->init->name, GFP_KERNEL);
 	if (!clk->name) {
 		pr_err("%s: could not allocate clk->name\n", __func__);
 		ret = -ENOMEM;
@@ -2075,7 +2075,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 
 	/* copy each string name in case parent_names is __initdata */
 	for (i = 0; i < clk->num_parents; i++) {
-		clk->parent_names[i] = kstrdup(hw->init->parent_names[i],
+		clk->parent_names[i] = kstrdup_const(hw->init->parent_names[i],
 						GFP_KERNEL);
 		if (!clk->parent_names[i]) {
 			pr_err("%s: could not copy parent_names\n", __func__);
@@ -2090,10 +2090,10 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 
 fail_parent_names_copy:
 	while (--i >= 0)
-		kfree(clk->parent_names[i]);
+		kfree_const(clk->parent_names[i]);
 	kfree(clk->parent_names);
 fail_parent_names:
-	kfree(clk->name);
+	kfree_const(clk->name);
 fail_name:
 	kfree(clk);
 fail_out:
@@ -2112,10 +2112,10 @@ static void __clk_release(struct kref *ref)
 
 	kfree(clk->parents);
 	while (--i >= 0)
-		kfree(clk->parent_names[i]);
+		kfree_const(clk->parent_names[i]);
 
 	kfree(clk->parent_names);
-	kfree(clk->name);
+	kfree_const(clk->name);
 	kfree(clk);
 }
 
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/5] clk: convert clock name allocations to kstrdup_const

[Mike Turquette]

Quoting Andrzej Hajda (2015-01-12 01:18:41)
> Clock subsystem frequently performs duplication of strings located
> in read-only memory section. Replacing kstrdup by kstrdup_const
> allows to avoid such operations.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Looks OK to me. Is there an easy trick to measuring the number of string
duplications saved short of instrumenting your code with a counter?

Regards,
Mike

> ---
>  drivers/clk/clk.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index f4963b7..27e644a 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2048,7 +2048,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>                 goto fail_out;
>         }
>  
> -       clk->name = kstrdup(hw->init->name, GFP_KERNEL);
> +       clk->name = kstrdup_const(hw->init->name, GFP_KERNEL);
>         if (!clk->name) {
>                 pr_err("%s: could not allocate clk->name\n", __func__);
>                 ret = -ENOMEM;
> @@ -2075,7 +2075,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>  
>         /* copy each string name in case parent_names is __initdata */
>         for (i = 0; i < clk->num_parents; i++) {
> -               clk->parent_names[i] = kstrdup(hw->init->parent_names[i],
> +               clk->parent_names[i] = kstrdup_const(hw->init->parent_names[i],
>                                                 GFP_KERNEL);
>                 if (!clk->parent_names[i]) {
>                         pr_err("%s: could not copy parent_names\n", __func__);
> @@ -2090,10 +2090,10 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>  
>  fail_parent_names_copy:
>         while (--i >= 0)
> -               kfree(clk->parent_names[i]);
> +               kfree_const(clk->parent_names[i]);
>         kfree(clk->parent_names);
>  fail_parent_names:
> -       kfree(clk->name);
> +       kfree_const(clk->name);
>  fail_name:
>         kfree(clk);
>  fail_out:
> @@ -2112,10 +2112,10 @@ static void __clk_release(struct kref *ref)
>  
>         kfree(clk->parents);
>         while (--i >= 0)
> -               kfree(clk->parent_names[i]);
> +               kfree_const(clk->parent_names[i]);
>  
>         kfree(clk->parent_names);
> -       kfree(clk->name);
> +       kfree_const(clk->name);
>         kfree(clk);
>  }
>  
> -- 
> 1.9.1
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/5] clk: convert clock name allocations to kstrdup_const

[Andrzej Hajda]

On 01/13/2015 12:11 AM, Mike Turquette wrote:
> Quoting Andrzej Hajda (2015-01-12 01:18:41)
>> Clock subsystem frequently performs duplication of strings located
>> in read-only memory section. Replacing kstrdup by kstrdup_const
>> allows to avoid such operations.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Looks OK to me. Is there an easy trick to measuring the number of string
> duplications saved short of instrumenting your code with a counter?

I have just added pr_err in kstrdup_const:

diff --git a/mm/util.c b/mm/util.c
index c96fc4b..32a97b2 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -56,8 +56,10 @@ EXPORT_SYMBOL(kstrdup);
 
 const char *kstrdup_const(const char *s, gfp_t gfp)
 {
-       if (is_kernel_rodata((unsigned long)s))
+       if (is_kernel_rodata((unsigned long)s)) {
+               pr_err("%s: %pS:%s\n", __func__,
__builtin_return_address(0), s);
                return s;
+       }
 
        return kstrdup(s, gfp);
 }

Probably printk buffer size should be increased:
CONFIG_LOG_BUF_SHIFT=17

Regards
Andrzej

>
> Regards,
> Mike
>
>> ---
>>  drivers/clk/clk.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index f4963b7..27e644a 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2048,7 +2048,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>>                 goto fail_out;
>>         }
>>  
>> -       clk->name = kstrdup(hw->init->name, GFP_KERNEL);
>> +       clk->name = kstrdup_const(hw->init->name, GFP_KERNEL);
>>         if (!clk->name) {
>>                 pr_err("%s: could not allocate clk->name\n", __func__);
>>                 ret = -ENOMEM;
>> @@ -2075,7 +2075,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>>  
>>         /* copy each string name in case parent_names is __initdata */
>>         for (i = 0; i < clk->num_parents; i++) {
>> -               clk->parent_names[i] = kstrdup(hw->init->parent_names[i],
>> +               clk->parent_names[i] = kstrdup_const(hw->init->parent_names[i],
>>                                                 GFP_KERNEL);
>>                 if (!clk->parent_names[i]) {
>>                         pr_err("%s: could not copy parent_names\n", __func__);
>> @@ -2090,10 +2090,10 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>>  
>>  fail_parent_names_copy:
>>         while (--i >= 0)
>> -               kfree(clk->parent_names[i]);
>> +               kfree_const(clk->parent_names[i]);
>>         kfree(clk->parent_names);
>>  fail_parent_names:
>> -       kfree(clk->name);
>> +       kfree_const(clk->name);
>>  fail_name:
>>         kfree(clk);
>>  fail_out:
>> @@ -2112,10 +2112,10 @@ static void __clk_release(struct kref *ref)
>>  
>>         kfree(clk->parents);
>>         while (--i >= 0)
>> -               kfree(clk->parent_names[i]);
>> +               kfree_const(clk->parent_names[i]);
>>  
>>         kfree(clk->parent_names);
>> -       kfree(clk->name);
>> +       kfree_const(clk->name);
>>         kfree(clk);
>>  }
>>  
>> -- 
>> 1.9.1
>>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4/5] mm/slab: convert cache name allocations to kstrdup_const

[Andrzej Hajda]

slab frequently performs duplication of strings located
in read-only memory section. Replacing kstrdup by kstrdup_const
allows to avoid such operations.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 mm/slab_common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index e03dd6f..2d94d1a 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -390,7 +390,7 @@ kmem_cache_create(const char *name, size_t size, size_t align,
 	if (s)
 		goto out_unlock;
 
-	cache_name = kstrdup(name, GFP_KERNEL);
+	cache_name = kstrdup_const(name, GFP_KERNEL);
 	if (!cache_name) {
 		err = -ENOMEM;
 		goto out_unlock;
@@ -401,7 +401,7 @@ kmem_cache_create(const char *name, size_t size, size_t align,
 				 flags, ctor, NULL, NULL);
 	if (IS_ERR(s)) {
 		err = PTR_ERR(s);
-		kfree(cache_name);
+		kfree_const(cache_name);
 	}
 
 out_unlock:
@@ -494,7 +494,7 @@ static int memcg_cleanup_cache_params(struct kmem_cache *s)
 
 void slab_kmem_cache_release(struct kmem_cache *s)
 {
-	kfree(s->name);
+	kfree_const(s->name);
 	kmem_cache_free(kmem_cache, s);
 }
 
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 5/5] fs/namespace: convert devname allocation to kstrdup_const

[Andrzej Hajda]

VFS frequently performs duplication of strings located
in read-only memory section. Replacing kstrdup by kstrdup_const
allows to avoid such operations.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 fs/namespace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index cd1e968..6dae553 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -201,7 +201,7 @@ static struct mount *alloc_vfsmnt(const char *name)
 			goto out_free_cache;
 
 		if (name) {
-			mnt->mnt_devname = kstrdup(name, GFP_KERNEL);
+			mnt->mnt_devname = kstrdup_const(name, GFP_KERNEL);
 			if (!mnt->mnt_devname)
 				goto out_free_id;
 		}
@@ -234,7 +234,7 @@ static struct mount *alloc_vfsmnt(const char *name)
 
 #ifdef CONFIG_SMP
 out_free_devname:
-	kfree(mnt->mnt_devname);
+	kfree_const(mnt->mnt_devname);
 #endif
 out_free_id:
 	mnt_free_id(mnt);
@@ -568,7 +568,7 @@ int sb_prepare_remount_readonly(struct super_block *sb)
 
 static void free_vfsmnt(struct mount *mnt)
 {
-	kfree(mnt->mnt_devname);
+	kfree_const(mnt->mnt_devname);
 #ifdef CONFIG_SMP
 	free_percpu(mnt->mnt_pcp);
 #endif
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] kstrdup optimization

[Geert Uytterhoeven]

On Mon, Jan 12, 2015 at 10:18 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> kstrdup if often used to duplicate strings where neither source neither
> destination will be ever modified. In such case we can just reuse the source
> instead of duplicating it. The problem is that we must be sure that
> the source is non-modifiable and its life-time is long enough.
>
> I suspect the good candidates for such strings are strings located in kernel
> .rodata section, they cannot be modifed because the section is read-only and
> their life-time is equal to kernel life-time.
>
> This small patchset proposes alternative version of kstrdup - kstrdup_const,
> which returns source string if it is located in .rodata otherwise it fallbacks
> to kstrdup.

It also introduces kfree_const(const void *x).

As kfree_const() has the exact same signature as kfree(), the risk of
accidentally passing pointers returned from kstrdup_const() to kfree() seems
high, which may lead to memory corruption if the pointer doesn't point to
allocated memory.

> To verify if the source is in .rodata function checks if the address is between
> sentinels __start_rodata, __end_rodata. I guess it should work with all
> architectures.
>
> The main patch is accompanied by four patches constifying kstrdup for cases
> where situtation described above happens frequently.
>
> As I have tested the patchset on mobile platform (exynos4210-trats) it saves
> 3272 string allocations. Since minimal allocation is 32 or 64 bytes depending
> on Kconfig options the patchset saves respectively about 100KB or 200KB of memory.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] kstrdup optimization

[Andrew Morton]

On Mon, 12 Jan 2015 21:45:58 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Mon, Jan 12, 2015 at 10:18 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> > kstrdup if often used to duplicate strings where neither source neither
> > destination will be ever modified. In such case we can just reuse the source
> > instead of duplicating it. The problem is that we must be sure that
> > the source is non-modifiable and its life-time is long enough.
> >
> > I suspect the good candidates for such strings are strings located in kernel
> > .rodata section, they cannot be modifed because the section is read-only and
> > their life-time is equal to kernel life-time.
> >
> > This small patchset proposes alternative version of kstrdup - kstrdup_const,
> > which returns source string if it is located in .rodata otherwise it fallbacks
> > to kstrdup.
> 
> It also introduces kfree_const(const void *x).
> 
> As kfree_const() has the exact same signature as kfree(), the risk of
> accidentally passing pointers returned from kstrdup_const() to kfree() seems
> high, which may lead to memory corruption if the pointer doesn't point to
> allocated memory.

Yes, it's an ugly little patchset.  But 100-200k of memory is hard to
argue with, and I'm not seeing a practical way of getting those savings
with a cleaner approach.

Hopefully a kfree(rodata-address) will promptly oops, but I haven't
tested that and it presumably depends on which flavour of
slab/sleb/slib/slob/slub you're using.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] kstrdup optimization

[Craig Milo Rogers]

> As kfree_const() has the exact same signature as kfree(), the risk of
> accidentally passing pointers returned from kstrdup_const() to kfree() seems
> high, which may lead to memory corruption if the pointer doesn't point to
> allocated memory.
...
>> To verify if the source is in .rodata function checks if the address is between
>> sentinels __start_rodata, __end_rodata. I guess it should work with all
>> architectures.

	kfree() could also check if the region being freed is in .rodata, and
ignore the call; kfree_const() would not be needed.  If making this check all
the time leads to a significant decrease in performance (numbers needed here),
another option is to keep kfree_const() but add a check to kfree(), when
compiled for debugging, that issues a suitable complaint if the region being
freed is in .rodata.

					Craig Milo Rogers

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] kstrdup optimization

[Andrew Morton]

On Tue, 13 Jan 2015 16:10:57 -0800 Craig Milo Rogers <rogers@isi.edu> wrote:

> > As kfree_const() has the exact same signature as kfree(), the risk of
> > accidentally passing pointers returned from kstrdup_const() to kfree() seems
> > high, which may lead to memory corruption if the pointer doesn't point to
> > allocated memory.
> ...
> >> To verify if the source is in .rodata function checks if the address is between
> >> sentinels __start_rodata, __end_rodata. I guess it should work with all
> >> architectures.
> 
> 	kfree() could also check if the region being freed is in .rodata, and
> ignore the call; kfree_const() would not be needed.  If making this check all
> the time leads to a significant decrease in performance (numbers needed here),
> another option is to keep kfree_const() but add a check to kfree(), when
> compiled for debugging, that issues a suitable complaint if the region being
> freed is in .rodata.
> 

Adding overhead to kfree() would be a show-stopper - it's a real
hotpath.

kstrdup_const() is only used in a small number of places.  Just don't
screw it up.


btw, I have vague memories that gcc used to put some strings into .text
under some circumstances.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] kstrdup optimization

[Andrew Morton]

On Mon, 12 Jan 2015 10:18:38 +0100 Andrzej Hajda <a.hajda@samsung.com> wrote:

> Hi,
> 
> kstrdup if often used to duplicate strings where neither source neither
> destination will be ever modified. In such case we can just reuse the source
> instead of duplicating it. The problem is that we must be sure that
> the source is non-modifiable and its life-time is long enough.
> 
> I suspect the good candidates for such strings are strings located in kernel
> .rodata section, they cannot be modifed because the section is read-only and
> their life-time is equal to kernel life-time.
> 
> This small patchset proposes alternative version of kstrdup - kstrdup_const,
> which returns source string if it is located in .rodata otherwise it fallbacks
> to kstrdup.
> To verify if the source is in .rodata function checks if the address is between
> sentinels __start_rodata, __end_rodata. I guess it should work with all
> architectures.
> 
> The main patch is accompanied by four patches constifying kstrdup for cases
> where situtation described above happens frequently.
> 
> As I have tested the patchset on mobile platform (exynos4210-trats) it saves
> 3272 string allocations. Since minimal allocation is 32 or 64 bytes depending
> on Kconfig options the patchset saves respectively about 100KB or 200KB of memory.

That's a lot of memory.  I wonder where it's all going to.  sysfs,
probably?

What the heck does (the cheerily undocumented) KERNFS_STATIC_NAME do
and can we remove it if this patchset is in place?


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] kstrdup optimization

[Andrzej Hajda]

On 01/14/2015 12:37 AM, Andrew Morton wrote:
> On Mon, 12 Jan 2015 10:18:38 +0100 Andrzej Hajda <a.hajda@samsung.com> wrote:
>
>> Hi,
>>
>> kstrdup if often used to duplicate strings where neither source neither
>> destination will be ever modified. In such case we can just reuse the source
>> instead of duplicating it. The problem is that we must be sure that
>> the source is non-modifiable and its life-time is long enough.
>>
>> I suspect the good candidates for such strings are strings located in kernel
>> .rodata section, they cannot be modifed because the section is read-only and
>> their life-time is equal to kernel life-time.
>>
>> This small patchset proposes alternative version of kstrdup - kstrdup_const,
>> which returns source string if it is located in .rodata otherwise it fallbacks
>> to kstrdup.
>> To verify if the source is in .rodata function checks if the address is between
>> sentinels __start_rodata, __end_rodata. I guess it should work with all
>> architectures.
>>
>> The main patch is accompanied by four patches constifying kstrdup for cases
>> where situtation described above happens frequently.
>>
>> As I have tested the patchset on mobile platform (exynos4210-trats) it saves
>> 3272 string allocations. Since minimal allocation is 32 or 64 bytes depending
>> on Kconfig options the patchset saves respectively about 100KB or 200KB of memory.
> That's a lot of memory.  I wonder where it's all going to.  sysfs,
> probably?

Stats from tested platform.
By caller:
  2260 __kernfs_new_node
    631 clk_register+0xc8/0x1b8
    318 clk_register+0x34/0x1b8
      51 kmem_cache_create
      12 alloc_vfsmnt

By string (with count >= 5):
    883 power
    876 subsystem
    135 parameters
    132 device
     61 iommu_group
     44 sclk_mpll
     42 aclk100
     41 driver
     36 sclk_vpll
     35 none
     34 sclk_epll
     34 aclk160
     32 sclk_hdmi24m
     31 xxti
     31 xusbxti
     31 sclk_usbphy0
     30 sclk_hdmiphy
     28 bdi
     28 aclk133
     14 sclk_apll
     14 aclk200
      9 module
      9 fin_pll
      5 div_core2
   


>
> What the heck does (the cheerily undocumented) KERNFS_STATIC_NAME do
> and can we remove it if this patchset is in place?
>
>

The only call path when this flag is set starts from
sysfs_add_file_mode_ns function.
But I guess this function can be called also for non-const names.

Regards
Andrzej


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] kstrdup optimization

[Tejun Heo]

On Tue, Jan 13, 2015 at 03:37:31PM -0800, Andrew Morton wrote:
> What the heck does (the cheerily undocumented) KERNFS_STATIC_NAME do
> and can we remove it if this patchset is in place?

The same thing, in a narrower scope.  It's currently used to avoid
making copies of sysfs file names which are required to stay unchanged
and accessible and usually allocated in the rodata section, but the
sysfs directory and all group file names are copied.  So, yeah, once
this is in, we can remove the explicit static name handling from
kernfs.

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>