On Wed, 9 Sep 2015 23:45:52 +0200 Rasmus Villemoes <[email protected]>
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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
