* [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
@ 2022-05-12 8:26 liqiong
2022-05-12 14:48 ` liqiong
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: liqiong @ 2022-05-12 8:26 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-mm, linux-kernel, liqiong
"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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
2022-05-12 8:26 [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]" liqiong
@ 2022-05-12 14:48 ` liqiong
2022-05-12 15:29 ` Muchun Song
2022-05-12 20:00 ` Andrew Morton
2 siblings, 0 replies; 8+ messages in thread
From: liqiong @ 2022-05-12 14:48 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-mm, linux-kernel, liqiong
"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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
2022-05-12 8:26 [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]" liqiong
2022-05-12 14:48 ` liqiong
@ 2022-05-12 15:29 ` Muchun Song
2022-05-12 17:10 ` liqiong
2022-05-12 20:00 ` Andrew Morton
2 siblings, 1 reply; 8+ messages in thread
From: Muchun Song @ 2022-05-12 15:29 UTC (permalink / raw)
To: liqiong; +Cc: Andrew Morton, linux-mm, linux-kernel
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
2022-05-12 15:29 ` Muchun Song
@ 2022-05-12 17:10 ` liqiong
0 siblings, 0 replies; 8+ messages in thread
From: liqiong @ 2022-05-12 17:10 UTC (permalink / raw)
To: Muchun Song; +Cc: Andrew Morton, linux-mm, linux-kernel
在 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楼
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
2022-05-12 8:26 [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]" liqiong
2022-05-12 14:48 ` liqiong
2022-05-12 15:29 ` Muchun Song
@ 2022-05-12 20:00 ` Andrew Morton
2022-05-13 6:34 ` liqiong
2022-05-13 11:06 ` David Laight
2 siblings, 2 replies; 8+ messages in thread
From: Andrew Morton @ 2022-05-12 20:00 UTC (permalink / raw)
To: liqiong; +Cc: linux-mm, linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
2022-05-12 20:00 ` Andrew Morton
@ 2022-05-13 6:34 ` liqiong
2022-05-13 11:06 ` David Laight
1 sibling, 0 replies; 8+ messages in thread
From: liqiong @ 2022-05-13 6:34 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-mm, linux-kernel
在 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
2022-05-12 20:00 ` Andrew Morton
2022-05-13 6:34 ` liqiong
@ 2022-05-13 11:06 ` David Laight
2022-05-13 14:27 ` Muchun Song
1 sibling, 1 reply; 8+ messages in thread
From: David Laight @ 2022-05-13 11:06 UTC (permalink / raw)
To: 'Andrew Morton', liqiong; +Cc: linux-mm, linux-kernel
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)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]"
2022-05-13 11:06 ` David Laight
@ 2022-05-13 14:27 ` Muchun Song
0 siblings, 0 replies; 8+ messages in thread
From: Muchun Song @ 2022-05-13 14:27 UTC (permalink / raw)
To: David Laight; +Cc: 'Andrew Morton', liqiong, linux-mm, linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-05-13 14:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-12 8:26 [PATCH] mm: change "char *bdi_unknown_name" to "char bdi_unknown_name[]" liqiong
2022-05-12 14:48 ` liqiong
2022-05-12 15:29 ` Muchun Song
2022-05-12 17:10 ` liqiong
2022-05-12 20:00 ` Andrew Morton
2022-05-13 6:34 ` liqiong
2022-05-13 11:06 ` David Laight
2022-05-13 14:27 ` Muchun Song
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.