On Thu, 12 May 2022 16:26:37 +0800 liqiong <[email protected]> wrote:
> "char bdi_unknown_nam[]" string form declares a single variable.
> It is better then "char *bdi_unknown_name" which creates two
> variables.
>
> ...
>
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -20,7 +20,7 @@ struct backing_dev_info noop_backing_dev_info;
> EXPORT_SYMBOL_GPL(noop_backing_dev_info);
>
> static struct class *bdi_class;
> -static const char *bdi_unknown_name = "(unknown)";
> +static const char bdi_unknown_name[] = "(unknown)";
>
heh, fun patch. We actually do this quite a lot.
grep -r "^[a-z].*char \*[a-z].*= \"" .
is a pathetic pattern which catches a lot of them.
However. I expected your patch to shrink the kernel a bit, but it has
the opposite effect:
hp2:/usr/src/25> size mm/backing-dev.o
text data bss dec hex filename
21288 9396 3808 34492 86bc mm/backing-dev.o-before
21300 9428 3808 34536 86e8 mm/backing-dev.o-after
Even .data became larger. I didn't investigate why.
在 2022年05月13日 04:00, Andrew Morton 写道:
> On Thu, 12 May 2022 16:26:37 +0800 liqiong <[email protected]> wrote:
>
>> "char bdi_unknown_nam[]" string form declares a single variable.
>> It is better then "char *bdi_unknown_name" which creates two
>> variables.
>>
>> ...
>>
>> --- a/mm/backing-dev.c
>> +++ b/mm/backing-dev.c
>> @@ -20,7 +20,7 @@ struct backing_dev_info noop_backing_dev_info;
>> EXPORT_SYMBOL_GPL(noop_backing_dev_info);
>>
>> static struct class *bdi_class;
>> -static const char *bdi_unknown_name = "(unknown)";
>> +static const char bdi_unknown_name[] = "(unknown)";
>>
> heh, fun patch. We actually do this quite a lot.
>
> grep -r "^[a-z].*char \*[a-z].*= \"" .
>
> is a pathetic pattern which catches a lot of them.
>
>
> However. I expected your patch to shrink the kernel a bit, but it has
> the opposite effect:
>
> hp2:/usr/src/25> size mm/backing-dev.o
> text data bss dec hex filename
> 21288 9396 3808 34492 86bc mm/backing-dev.o-before
> 21300 9428 3808 34536 86e8 mm/backing-dev.o-after
>
> Even .data became larger. I didn't investigate why.
Hi,
It seems the patch creates a new section:
0000000000000000 l ___ksymtab_gpl+bdi_dev_name 0000000000000000 __ksymtab_bdi_dev_name
0000000000000f60 l O .rodata 000000000000000a bdi_unknown_name
And put "bdi_unknown_name" at .rodata.str1.1:
This work was published in "KernelJanitors/Todo". It says:
"foo []" is better because it declares a single variable, For variables marked __initdata, the "*foo" form causes
only the pointer, not the string itself, to be dropped from the kernel image, which is a bug.
Using the "foo[]" form with regular 'ole local variables also makes the assembly shorter.
I thought it make sense, so i searched the mm tree by "grep -nHre char.*\*.*=.*\"", and checked all the "char *foo"
string form, It seems only this one should be fixed.
From: Andrew Morton
> Sent: 12 May 2022 21:01
>
> On Thu, 12 May 2022 16:26:37 +0800 liqiong <[email protected]> wrote:
>
> > "char bdi_unknown_nam[]" string form declares a single variable.
> > It is better then "char *bdi_unknown_name" which creates two
> > variables.
> >
> > ...
> >
> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
> > @@ -20,7 +20,7 @@ struct backing_dev_info noop_backing_dev_info;
> > EXPORT_SYMBOL_GPL(noop_backing_dev_info);
> >
> > static struct class *bdi_class;
> > -static const char *bdi_unknown_name = "(unknown)";
> > +static const char bdi_unknown_name[] = "(unknown)";
> >
>
> heh, fun patch. We actually do this quite a lot.
>
> grep -r "^[a-z].*char \*[a-z].*= \"" .
>
> is a pathetic pattern which catches a lot of them.
>
>
> However. I expected your patch to shrink the kernel a bit, but it has
> the opposite effect:
>
> hp2:/usr/src/25> size mm/backing-dev.o
> text data bss dec hex filename
> 21288 9396 3808 34492 86bc mm/backing-dev.o-before
> 21300 9428 3808 34536 86e8 mm/backing-dev.o-after
>
> Even .data became larger. I didn't investigate why.
The linker can merge replicated strings
(ie data in .rodata.str1.n sections)
but I don't think the compiler puts variables into that section.
So if you have:
static const char *const foo_xxx = "foo";
in multiple source/object files you get lots of pointers
but only one string.
OTOH with:
static const char foo_xxx[] = "foo";
you get lots of copies of the string.
Which is smaller depends on the number of variables and the length
of the string.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, May 13, 2022 at 11:06:23AM +0000, David Laight wrote:
> From: Andrew Morton
> > Sent: 12 May 2022 21:01
> >
> > On Thu, 12 May 2022 16:26:37 +0800 liqiong <[email protected]> wrote:
> >
> > > "char bdi_unknown_nam[]" string form declares a single variable.
> > > It is better then "char *bdi_unknown_name" which creates two
> > > variables.
> > >
> > > ...
> > >
> > > --- a/mm/backing-dev.c
> > > +++ b/mm/backing-dev.c
> > > @@ -20,7 +20,7 @@ struct backing_dev_info noop_backing_dev_info;
> > > EXPORT_SYMBOL_GPL(noop_backing_dev_info);
> > >
> > > static struct class *bdi_class;
> > > -static const char *bdi_unknown_name = "(unknown)";
> > > +static const char bdi_unknown_name[] = "(unknown)";
> > >
> >
> > heh, fun patch. We actually do this quite a lot.
> >
> > grep -r "^[a-z].*char \*[a-z].*= \"" .
> >
> > is a pathetic pattern which catches a lot of them.
> >
> >
> > However. I expected your patch to shrink the kernel a bit, but it has
> > the opposite effect:
> >
> > hp2:/usr/src/25> size mm/backing-dev.o
> > text data bss dec hex filename
> > 21288 9396 3808 34492 86bc mm/backing-dev.o-before
> > 21300 9428 3808 34536 86e8 mm/backing-dev.o-after
> >
> > Even .data became larger. I didn't investigate why.
>
> The linker can merge replicated strings
> (ie data in .rodata.str1.n sections)
> but I don't think the compiler puts variables into that section.
>
> So if you have:
> static const char *const foo_xxx = "foo";
> in multiple source/object files you get lots of pointers
> but only one string.
> OTOH with:
> static const char foo_xxx[] = "foo";
> you get lots of copies of the string.
> Which is smaller depends on the number of variables and the length
> of the string.
>
Good point. I have searched the whole code. There are 19 places
where use the string "(unknown)". Seems it is better to drop
this change.
arch/mips/alchemy/devboards/db1xxx.c:48: return "(unknown)";
drivers/acpi/device_pm.c:44: return "(unknown)";
drivers/base/component.c:101: component ? dev_name(component->dev) : "(unknown)",
drivers/block/rbd.c:5137: spec->image_id, spec->image_name ?: "(unknown)",
drivers/gpu/drm/i915/display/intel_dsi_vbt.c:570: return "(unknown)";
drivers/i3c/master/mipi-i3c-hci/ext_caps.c:50: "(unknown)", "master only", "target only",
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.c:85: return "(unknown)";
drivers/platform/x86/thinkpad_acpi.c:7506: thinkpad_id.ec_version_str : "(unknown)");
drivers/usb/gadget/udc/omap_udc.c:2800: type = "(unknown)";
fs/cifs/cifs_swn.c:653: seq_puts(m, "(unknown)");
fs/cifs/cifsfs.c:434: seq_puts(s, "(unknown)");
fs/ext4/super.c:835: path = "(unknown)";
include/drm/drm_mode_object.h:118: return "(unknown)"; \
lib/error-inject.c:187: return "(unknown)";
mm/backing-dev.c:23:static const char *bdi_unknown_name = "(unknown)";
mm/filemap.c:3664: path = "(unknown)";
net/ipv4/cipso_ipv4.c:444: type_str = "(unknown)";
net/ipv6/calipso.c:384: type_str = "(unknown)";
sound/firewire/dice/dice-proc.c:35: return "(unknown)";
Thanks.