Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 13 May 2022 22:27:47 +0800
From:   Muchun Song <songmuchun@bytedance.com>
To:     David Laight <David.Laight@ACULAB.COM>
Cc:     'Andrew Morton' <akpm@linux-foundation.org>,
        liqiong <liqiong@nfschina.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] mm: change "char *bdi_unknown_name" to "char
 bdi_unknown_name[]"
Message-ID: <Yn5q43kblRPTvpDD@FVFYT0MHHV2J.usts.net>
References: <20220512082637.24649-1-liqiong@nfschina.com>
 <20220512130051.94a0c53e5d1498292473975d@linux-foundation.org>
 <4b21dec7e98243b89daea96286c33434@AcuMS.aculab.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4b21dec7e98243b89daea96286c33434@AcuMS.aculab.com>
Precedence: bulk

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.