On Mon, Sep 14 2015, Andrew Morton <[email protected]> wrote:
> On Wed, 9 Sep 2015 23:45:52 +0200 Rasmus Villemoes > <[email protected]> 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
