Messages in this thread | | | Date | Mon, 14 Sep 2015 13:42:05 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 2/2] kobject: use kvasprintf_const for formatting ->name |
| |
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;
| |