Messages in this thread | | | From | Rasmus Villemoes <> | Subject | Re: [PATCH 2/2] kobject: use kvasprintf_const for formatting ->name | Date | Tue, 15 Sep 2015 01:33:30 +0200 |
| |
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
| |