[PATCH 1/2] lib: introduce kvasprintf_const

[PATCH 1/2] lib: introduce kvasprintf_const

[Rasmus Villemoes]

This adds kvasprintf_const which tries to use kstrdup_const if
possible: If the format string contains no % characters, or if the
format string is exactly "%s", we delegate to
kstrdup_const. Otherwise, we fall back to kvasprintf.

Just as for kstrdup_const, the main motivation is to save memory by
reusing .rodata when possible.

The return value should be freed by kfree_const, just like for
kstrdup_const.

There is deliberately no kasprintf_const: In the vast majority of
cases, the format string argument is a literal, so one can determine
statically whether one could instead use kstrdup_const directly (which
would also require one to change all corresponding kfree calls to
kfree_const).

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 include/linux/kernel.h |  2 ++
 lib/kasprintf.c        | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 5582410727cb..2c13f747ac2e 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -413,6 +413,8 @@ extern __printf(2, 3)
 char *kasprintf(gfp_t gfp, const char *fmt, ...);
 extern __printf(2, 0)
 char *kvasprintf(gfp_t gfp, const char *fmt, va_list args);
+extern __printf(2, 0)
+const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
 
 extern __scanf(2, 3)
 int sscanf(const char *, const char *, ...);
diff --git a/lib/kasprintf.c b/lib/kasprintf.c
index 32f12150fc4f..f194e6e593e1 100644
--- a/lib/kasprintf.c
+++ b/lib/kasprintf.c
@@ -31,6 +31,22 @@ char *kvasprintf(gfp_t gfp, const char *fmt, va_list ap)
 }
 EXPORT_SYMBOL(kvasprintf);
 
+/*
+ * If fmt contains no % (or is exactly %s), use kstrdup_const. If fmt
+ * (or the sole vararg) points to rodata, we will then save a memory
+ * allocation and string copy. In any case, the return value should be
+ * freed using kfree_const().
+ */
+const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list ap)
+{
+	if (!strchr(fmt, '%'))
+		return kstrdup_const(fmt, gfp);
+	if (!strcmp(fmt, "%s"))
+		return kstrdup_const(va_arg(ap, const char*), gfp);
+	return kvasprintf(gfp, fmt, ap);
+}
+EXPORT_SYMBOL(kvasprintf_const);
+
 char *kasprintf(gfp_t gfp, const char *fmt, ...)
 {
 	va_list ap;
-- 
2.1.3

[PATCH 2/2] kobject: use kvasprintf_const for formatting ->name

[Rasmus Villemoes]

Sometimes kobject_set_name_vargs is called with a format string
conaining no %, or a format string of precisely "%s", where the single
vararg happens to point to .rodata. kvasprintf_const detects these
cases for us and returns a copy of that pointer instead of duplicating
the string, thus saving some run-time memory. Otherwise, it falls back
to kvasprintf. We just need to always deallocate ->name using
kfree_const.

Unfortunately, the dance we need to do to perform the '/' -> '!'
sanitization makes the resulting code rather ugly.

I instrumented kstrdup_const to provide some statistics on the memory
saved, and for me this gave an additional ~14KB after boot (306KB was
already saved; this patch bumped that to 320KB). I have
KMALLOC_SHIFT_LOW==3, and since 80% of the kvasprintf_const hits were
satisfied by an 8-byte allocation, the 14K would roughly be quadrupled
when KMALLOC_SHIFT_LOW==5. Whether these numbers are sufficient to
justify the ugliness I'll leave to others to decide.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/kobject.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/lib/kobject.c b/lib/kobject.c
index 3e3a5c3cb330..fee2fd950306 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -257,18 +257,32 @@ static int kobject_add_internal(struct kobject *kobj)
 int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
 				  va_list vargs)
 {
-	char *s;
+	const char *s;
 
 	if (kobj->name && !fmt)
 		return 0;
 
-	s = kvasprintf(GFP_KERNEL, fmt, vargs);
+	s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
 	if (!s)
 		return -ENOMEM;
 
-	/* ewww... some of these buggers have '/' in the name ... */
-	strreplace(s, '/', '!');
-	kfree(kobj->name);
+	/*
+	 * ewww... some of these buggers have '/' in the name ... If
+	 * that's the case, we need to make sure we have an actual
+	 * allocated copy to modify, since kvasprintf_const may have
+	 * returned something from .rodata.
+	 */
+	if (strchr(s, '/')) {
+		char *t;
+
+		t = kstrdup(s, GFP_KERNEL);
+		kfree_const(s);
+		if (!t)
+			return -ENOMEM;
+		strreplace(t, '/', '!');
+		s = t;
+	}
+	kfree_const(kobj->name);
 	kobj->name = s;
 
 	return 0;
@@ -466,7 +480,7 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
 	envp[0] = devpath_string;
 	envp[1] = NULL;
 
-	name = dup_name = kstrdup(new_name, GFP_KERNEL);
+	name = dup_name = kstrdup_const(new_name, GFP_KERNEL);
 	if (!name) {
 		error = -ENOMEM;
 		goto out;
@@ -486,7 +500,7 @@ int kobject_rename(struct kobject *kobj, const char *new_name)
 	kobject_uevent_env(kobj, KOBJ_MOVE, envp);
 
 out:
-	kfree(dup_name);
+	kfree_const(dup_name);
 	kfree(devpath_string);
 	kfree(devpath);
 	kobject_put(kobj);
@@ -632,7 +646,7 @@ static void kobject_cleanup(struct kobject *kobj)
 	/* free name if we allocated it */
 	if (name) {
 		pr_debug("kobject: '%s': free name\n", name);
-		kfree(name);
+		kfree_const(name);
 	}
 }
 
-- 
2.1.3

Re: [PATCH 2/2] kobject: use kvasprintf_const for formatting ->name

[Andrew Morton]

On Wed,  9 Sep 2015 23:45:52 +0200 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> Sometimes kobject_set_name_vargs is called with a format string
> conaining no %, or a format string of precisely "%s", where the single
> vararg happens to point to .rodata. kvasprintf_const detects these
> cases for us and returns a copy of that pointer instead of duplicating
> the string, thus saving some run-time memory. Otherwise, it falls back
> to kvasprintf. We just need to always deallocate ->name using
> kfree_const.
> 
> Unfortunately, the dance we need to do to perform the '/' -> '!'
> sanitization makes the resulting code rather ugly.
> 
> I instrumented kstrdup_const to provide some statistics on the memory
> saved, and for me this gave an additional ~14KB after boot (306KB was
> already saved; this patch bumped that to 320KB). I have
> KMALLOC_SHIFT_LOW==3, and since 80% of the kvasprintf_const hits were
> satisfied by an 8-byte allocation, the 14K would roughly be quadrupled
> when KMALLOC_SHIFT_LOW==5. Whether these numbers are sufficient to
> justify the ugliness I'll leave to others to decide.

Do we have other callsites whcih can benefit from switching to
kvasprintf_const()?  The [1/2] changelog didn't make this clear.

> 
> diff --git a/lib/kobject.c b/lib/kobject.c
> index 3e3a5c3cb330..fee2fd950306 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -257,18 +257,32 @@ static int kobject_add_internal(struct kobject *kobj)
>  int kobject_set_name_vargs(struct kobject *kobj, const char *fmt,
>  				  va_list vargs)
>  {
> -	char *s;
> +	const char *s;
>  
>  	if (kobj->name && !fmt)
>  		return 0;
>  
> -	s = kvasprintf(GFP_KERNEL, fmt, vargs);
> +	s = kvasprintf_const(GFP_KERNEL, fmt, vargs);
>  	if (!s)
>  		return -ENOMEM;
>  
> -	/* ewww... some of these buggers have '/' in the name ... */
> -	strreplace(s, '/', '!');
> -	kfree(kobj->name);
> +	/*
> +	 * ewww... some of these buggers have '/' in the name ... If
> +	 * that's the case, we need to make sure we have an actual
> +	 * allocated copy to modify, since kvasprintf_const may have
> +	 * returned something from .rodata.
> +	 */
> +	if (strchr(s, '/')) {

It doesn't look too ugly to me.

Can we test here whether kvasprintf_const() really returned somethnig
in .rodata?

> +		char *t;
> +
> +		t = kstrdup(s, GFP_KERNEL);
> +		kfree_const(s);
> +		if (!t)
> +			return -ENOMEM;
> +		strreplace(t, '/', '!');
> +		s = t;
> +	}
> +	kfree_const(kobj->name);
>  	kobj->name = s;
>  
>  	return 0;

Re: [PATCH 2/2] kobject: use kvasprintf_const for formatting ->name

[Rasmus Villemoes]

On Mon, Sep 14 2015, Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed,  9 Sep 2015 23:45:52 +0200 Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
> Do we have other callsites whcih can benefit from switching to
> kvasprintf_const()?  The [1/2] changelog didn't make this clear.

There are a few, but kobject_set_name_vargs was the only I found that
would give KB savings (at least for my setup). Finding all places the
return value is freed and switch over to kfree_const is a little
work, and there's often also some struct member to change from "char*" to
"const char*" with a little fallout. IMHO, such constifications
would count as nice side effects, but I could also see why some might
think of it as useless churn.

A few candidates:

drivers/gpu/drm/drm_drv.c: drm_dev_set_unique()

drivers/xen/xenbus/xenbus_xs.c: xenbus_printf(): This one's easy, as the
kvasprintf return value is local to the function. But that also means we
wouldn't save any longterm memory. Many callers pass a format of "%d"
and then a literal 0 or 1, so they could be changed to passing "0" or
"1", saving a tiny bit of .text and a few cycles.

sound/pci/hda/hda_codec.c: snd_hda_codec_pcm_new(): Most callers pass
literals or "%s". I'm pretty sure the only kfree function to change is
the one in release_pcm a few lines above, so the biggest problem would
be changing struct hda_pcm->name to const char* and fixing the fallout
from that.

> It doesn't look too ugly to me.
>
> Can we test here whether kvasprintf_const() really returned somethnig
> in .rodata?

Yeah, I also thought about avoiding kstrdup() if we already have a
modifiable string. We'd have to move is_kernel_rodata to some header
(I'd say a new one, linux/sections.h, which could then include
asm/sections.h for the declarations of __start_rodata,
__end_rodata). And then I'd move this block to a small helper and do

s = sanitize_slashes(s);
if (!s)
	return -ENOMEM;

or something. sanitize_slashes would be

char *t;

if (!strchr(s, '/))
	return s;
if (is_kernel_rodata(s)) {
	t = kstrdup(s, GFP_KERNEL);
	if (!t)
		return NULL;
} else {
	t = (char*)s;
}
strreplace(t, '/', '!');
return t;

But maybe that's knowing too much about how
kvasprintf_const/kstrdup_const work (for example, it would be bad if
they ever learned another unmodifiable section). In any case, would be
better as one or two follow-up patches.

Rasmus