Re: [PATCH v3] export.h: reduce __ksymtab_strings string duplication by using "MS" section flags

[Jessica Yu]

+++ Masahiro Yamada [12/12/19 15:22 +0900]:
> On Fri, Dec 6, 2019 at 9:41 PM Jessica Yu <jeyu@xxxxxxxxxx> wrote:
> > Commit c3a6cf19e695 ("export: avoid code duplication in
> > include/linux/export.h") refactors export.h quite nicely, but introduces
> > a slight increase in memory usage due to using the empty string ""
> > instead of NULL to indicate that an exported symbol has no namespace. As
> > mentioned in that commit, this meant an increase of 1 byte per exported
> > symbol without a namespace. For example, if a kernel configuration has
> > about 10k exported symbols, this would mean that the size of
> > __ksymtab_strings would increase by roughly 10kB.
> >
> > We can alleviate this situation by utilizing the SHF_MERGE and
> > SHF_STRING section flags. SHF_MERGE|SHF_STRING indicate to the linker
> > that the data in the section are null-terminated strings that can be
> > merged to eliminate duplication. More specifically, from the binutils
> > documentation - "for sections with both M and S, a string which is a
> > suffix of a larger string is considered a duplicate. Thus "def" will be
> > merged with "abcdef"; A reference to the first "def" will be changed to
> > a reference to "abcdef"+3". Thus, all the empty strings would be merged
> > as well as any strings that can be merged according to the cited method
> > above. For example, "memset" and "__memset" would be merged to just
> > "__memset" in __ksymtab_strings.
> >
> > As of v5.4-rc5, the following statistics were gathered with x86
> > defconfig with approximately 10.7k exported symbols.
> >
> > Size of __ksymtab_strings in vmlinux:
> > -------------------------------------
> > v5.4-rc5: 213834 bytes
> > v5.4-rc5 with commit c3a6cf19e695: 224455 bytes
> > v5.4-rc5 with this patch: 205759 bytes
> >
> > So, we already see memory savings of ~8kB compared to vanilla -rc5 and
> > savings of nearly 18.7kB compared to -rc5 with commit c3a6cf19e695 on top.
> >
> > Unfortunately, as of this writing, strings will not get deduplicated for
> > kernel modules, as ld does not do the deduplication for
> > SHF_MERGE|SHF_STRINGS sections for relocatable files (ld -r), which
> > kernel modules are. A patch for ld is currently being worked on to
> > hopefully allow for string deduplication in relocatable files in the
> > future.
> >
> > Suggested-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Jessica Yu <jeyu@xxxxxxxxxx>
> > ---
> > v3:
> >   - remove __KSTRTAB_ENTRY macros in favor of just putting the asm directly
> >     in ___EXPORT_SYMBOL
> >   - Document more clearly what the ___EXPORT_SYMBOL macro does
> >
> >  include/asm-generic/export.h |  8 +++++---
> >  include/linux/export.h       | 27 ++++++++++++++++++++-------
> >  2 files changed, 25 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
> > index fa577978fbbd..23bc98e97a66 100644
> > --- a/include/asm-generic/export.h
> > +++ b/include/asm-generic/export.h
> > @@ -26,9 +26,11 @@
> >  .endm
> >
> >  /*
> > - * note on .section use: @progbits vs %progbits nastiness doesn't matter,
> > - * since we immediately emit into those sections anyway.
> > + * note on .section use: we specify progbits since usage of the "M" (SHF_MERGE)
> > + * section flag requires it. Use '%progbits' instead of '@progbits' since the
> > + * former apparently works on all arches according to the binutils source.
> >   */
> > +
> >  .macro ___EXPORT_SYMBOL name,val,sec
> >  #ifdef CONFIG_MODULES
> >         .globl __ksymtab_\name
> > @@ -37,7 +39,7 @@
> >  __ksymtab_\name:
> >         __put \val, __kstrtab_\name
> >         .previous
> > -       .section __ksymtab_strings,"a"
> > +       .section __ksymtab_strings,"aMS",%progbits,1
> >  __kstrtab_\name:
> >         .asciz "\name"
> >         .previous
> > diff --git a/include/linux/export.h b/include/linux/export.h
> > index 201262793369..18dcdcd118e7 100644
> > --- a/include/linux/export.h
> > +++ b/include/linux/export.h
> > @@ -81,16 +81,29 @@ struct kernel_symbol {
> >
> >  #else
> >
> > -/* For every exported symbol, place a struct in the __ksymtab section */
> > +/*
> > + * For every exported symbol, do the following:
>
> Just a nit.
>
> You mention "an entry" twice
> to talk about different classes of structures.
>
> It might be better to be explicit about "which entry".
>
>
> > + *
> > + * - If applicable, place an entry in the __kcrctab section.
>
>  "place a CRC entry in the __kcrctab section"
> might be clearer ?
>
>
> > + * - Put the name of the symbol and namespace (empty string "" for none) in
> > + *   __ksymtab_strings.
> > + * - Place an entry in the __ksymtab section.
>
> "Place a struct kernel_symbol entry in __ksymtab section"   ?
> might be clearer ?
>
>
> Other than that:
> Reviewed-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>

Yeah, that is much clearer, will resend. Thanks! 

Jessica