Re: [PATCH 2/2] kobject: use kvasprintf_const for formatting ->name

[Rasmus Villemoes]

On Mon, Sep 14 2015, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Wed,  9 Sep 2015 23:45:52 +0200 Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> 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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/