From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751908AbbINXde (ORCPT ); Mon, 14 Sep 2015 19:33:34 -0400 Received: from mail-wi0-f171.google.com ([209.85.212.171]:33832 "EHLO mail-wi0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751198AbbINXdd (ORCPT ); Mon, 14 Sep 2015 19:33:33 -0400 From: Rasmus Villemoes To: Andrew Morton Cc: Greg Kroah-Hartman , linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] kobject: use kvasprintf_const for formatting ->name Organization: D03 References: <1441835154-14560-1-git-send-email-linux@rasmusvillemoes.dk> <1441835154-14560-2-git-send-email-linux@rasmusvillemoes.dk> <20150914134205.a9d926185de33ddfd1b48808@linux-foundation.org> X-Hashcash: 1:20:150914:gregkh@linuxfoundation.org::Apw+s5AK9xkRmQMu:000000000000000000000000000000000003MbS X-Hashcash: 1:20:150914:linux-kernel@vger.kernel.org::4qDxxL8IY/OzqtHf:00000000000000000000000000000000040s0 X-Hashcash: 1:20:150914:akpm@linux-foundation.org::oKDslw62CNaetP9k:0000000000000000000000000000000000007GTj Date: Tue, 15 Sep 2015 01:33:30 +0200 In-Reply-To: <20150914134205.a9d926185de33ddfd1b48808@linux-foundation.org> (Andrew Morton's message of "Mon, 14 Sep 2015 13:42:05 -0700") Message-ID: <87lhc8wp85.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 14 2015, Andrew Morton wrote: > On Wed, 9 Sep 2015 23:45:52 +0200 Rasmus Villemoes 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