Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191120145110.8397-1-jeyu@kernel.org> <20191125154217.18640-1-jeyu@kernel.org>
In-Reply-To: <20191125154217.18640-1-jeyu@kernel.org>
From:   Masahiro Yamada <yamada.masahiro@socionext.com>
Date:   Tue, 26 Nov 2019 17:32:59 +0900
Message-ID: <CAK7LNASU9YysYNXuBKSU4WeUyE=2itfLDYzCupXL-49GUZuGnQ@mail.gmail.com>
Subject: Re: [PATCH v2] export.h: reduce __ksymtab_strings string duplication
 by using "MS" section flags
To:     Jessica Yu <jeyu@kernel.org>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Matthias Maennich <maennich@google.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 26, 2019 at 12:42 AM Jessica Yu <jeyu@kernel.org> 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@rasmusvillemoes.dk>
> Signed-off-by: Jessica Yu <jeyu@kernel.org>
> ---
>
> v2: use %progbits throughout and document the oddity in a comment.
>
>  include/asm-generic/export.h |  8 +++++---
>  include/linux/export.h       | 27 +++++++++++++++++++++------
>  2 files changed, 26 insertions(+), 9 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..3d835ca34d33 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -81,16 +81,31 @@ struct kernel_symbol {
>
>  #else
>
> +/*
> + * 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.
> + */
> +#define __KSTRTAB_ENTRY(sym)                                                   \
> +       asm("   .section \"__ksymtab_strings\",\"aMS\",%progbits,1      \n"     \
> +           "__kstrtab_" #sym ":                                        \n"     \
> +           "   .asciz  \"" #sym "\"                                    \n"     \
> +           "   .previous                                               \n")
> +
> +#define __KSTRTAB_NS_ENTRY(sym, ns)                                            \
> +       asm("   .section \"__ksymtab_strings\",\"aMS\",%progbits,1      \n"     \
> +           "__kstrtabns_" #sym ":                                      \n"     \
> +           "   .asciz  " #ns "                                         \n"     \


Hmm, it took some time for me to how this code works.

ns is already a C string, then you added # to it,
then I was confused.

Personally, I prefer this code:
" .asciz \"" ns "\" \n"

so it looks in the same way as __KSTRTAB_ENTRY().


BTW, you duplicated \"aMS\",%progbits,1" and ".previous"


I would write it shorter, like this:


#define ___EXPORT_SYMBOL(sym, sec, ns) \
        extern typeof(sym) sym; \
        extern const char __kstrtab_##sym[]; \
        extern const char __kstrtabns_##sym[]; \
        __CRC_SYMBOL(sym, sec); \
        asm("    .section \"__ksymtab_strings\",\"aMS\",%progbits,1\n" \
            "__kstrtab_" #sym ": \n" \
            "     .asciz \"" #sym "\" \n" \
            "__kstrtabns_" #sym ": \n" \
            "     .asciz \"" ns "\" \n" \
            "     .previous \n");    \
       __KSYMTAB_ENTRY(sym, sec)


> +           "   .previous                                               \n")
> +
>  /* For every exported symbol, place a struct in the __ksymtab section */
>  #define ___EXPORT_SYMBOL(sym, sec, ns)                                 \
>         extern typeof(sym) sym;                                         \
> +       extern const char __kstrtab_##sym[];                            \
> +       extern const char __kstrtabns_##sym[];                          \
>         __CRC_SYMBOL(sym, sec);                                         \
> -       static const char __kstrtab_##sym[]                             \
> -       __attribute__((section("__ksymtab_strings"), used, aligned(1))) \
> -       = #sym;                                                         \
> -       static const char __kstrtabns_##sym[]                           \
> -       __attribute__((section("__ksymtab_strings"), used, aligned(1))) \
> -       = ns;                                                           \
> +       __KSTRTAB_ENTRY(sym);                                           \
> +       __KSTRTAB_NS_ENTRY(sym, ns);                                    \
>         __KSYMTAB_ENTRY(sym, sec)
>
>  #endif
> --
> 2.16.4
>


-- 
Best Regards
Masahiro Yamada