From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751725AbbINUmJ (ORCPT ); Mon, 14 Sep 2015 16:42:09 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:42325 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750951AbbINUmG (ORCPT ); Mon, 14 Sep 2015 16:42:06 -0400 Date: Mon, 14 Sep 2015 13:42:05 -0700 From: Andrew Morton To: Rasmus Villemoes Cc: Greg Kroah-Hartman , linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] kobject: use kvasprintf_const for formatting ->name Message-Id: <20150914134205.a9d926185de33ddfd1b48808@linux-foundation.org> In-Reply-To: <1441835154-14560-2-git-send-email-linux@rasmusvillemoes.dk> References: <1441835154-14560-1-git-send-email-linux@rasmusvillemoes.dk> <1441835154-14560-2-git-send-email-linux@rasmusvillemoes.dk> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 9 Sep 2015 23:45:52 +0200 Rasmus Villemoes 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;