1. Patchew
  2. linux
  3. View series

[PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"

liqiong posted 1 patch 3 years, 12 months ago
There is a newer version of this series
mm/backing-dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
Posted by liqiong 3 years, 12 months ago 
"char bdi_unknown_nam[]" string form declares a single variable.
It is better then "char *bdi_unknown_name" which creates two
variables.

Signed-off-by: liqiong <liqiong@nfschina.com>
---
 mm/backing-dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 7176af65b103..4982ccc63536 100644
--- 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)";
 
 /*
  * bdi_lock protects bdi_tree and updates to bdi_list. bdi_list has RCU
-- 
2.11.0
Re: [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
Posted by Andrew Morton 3 years, 12 months ago 
On Thu, 12 May 2022 16:26:37 +0800 liqiong <liqiong@nfschina.com> 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.
RE: [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
Posted by David Laight 3 years, 11 months ago 
From: Andrew Morton
> Sent: 12 May 2022 21:01
> 
> On Thu, 12 May 2022 16:26:37 +0800 liqiong <liqiong@nfschina.com> 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)
Re: [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
Posted by Muchun Song 3 years, 11 months ago 
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 <liqiong@nfschina.com> 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.
Re: [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
Posted by liqiong 3 years, 11 months ago 
在 2022年05月13日 04:00, Andrew Morton 写道:
> On Thu, 12 May 2022 16:26:37 +0800 liqiong <liqiong@nfschina.com> 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.
Re: [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
Posted by Muchun Song 3 years, 12 months ago 
On Thu, May 12, 2022 at 04:26:37PM +0800, liqiong wrote:
> "char bdi_unknown_nam[]" string form declares a single variable.
> It is better then "char *bdi_unknown_name" which creates two
> variables.
>

Sorry, I do not understand what you are saying here.  Creating
two variables means what?

Thanks.
 
> Signed-off-by: liqiong <liqiong@nfschina.com>
> ---
>  mm/backing-dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 7176af65b103..4982ccc63536 100644
> --- 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)";
>  
>  /*
>   * bdi_lock protects bdi_tree and updates to bdi_list. bdi_list has RCU
> -- 
> 2.11.0
> 
>
Re: [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
Posted by liqiong 3 years, 12 months ago 

在 2022年05月12日 23:29, Muchun Song 写道:
> On Thu, May 12, 2022 at 04:26:37PM +0800, liqiong wrote:
>> "char bdi_unknown_nam[]" string form declares a single variable.
>> It is better then "char *bdi_unknown_name" which creates two
>> variables.
>>
> Sorry, I do not understand what you are saying here.  Creating
> two variables means what?
>
> Thanks.

Hi there，

The string form of "char *" creates two variables in the final assembly output, 
a static string, and a char pointer to the static string.

Use "objdump -S -D *.o", can find out the static string occurring at 
"Contents of section .rodata".


>  
>> Signed-off-by: liqiong <liqiong@nfschina.com>
>> ---
>>  mm/backing-dev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
>> index 7176af65b103..4982ccc63536 100644
>> --- 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)";
>>  
>>  /*
>>   * bdi_lock protects bdi_tree and updates to bdi_list. bdi_list has RCU
>> -- 
>> 2.11.0
>>
>>

-- 
李力琼 <13524287433>
上海市浦东新区海科路99号中科院上海高等研究院3号楼3楼
[PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
Posted by liqiong 3 years, 12 months ago 
"char bdi_unknown_nam[]" string form declares a single variable.
It is better than "char *bdi_unknown_name" which creates two
variables.

Signed-off-by: liqiong <liqiong@nfschina.com>
---
 mm/backing-dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 7176af65b103..4982ccc63536 100644
--- 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)";
 
 /*
  * bdi_lock protects bdi_tree and updates to bdi_list. bdi_list has RCU
-- 
2.11.0

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.