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;
Date:   Tue, 26 Nov 2019 16:02:09 +0000
From:   Matthias Maennich <maennich@google.com>
To:     Jessica Yu <jeyu@kernel.org>
Cc:     Masahiro Yamada <yamada.masahiro@socionext.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v2] export.h: reduce __ksymtab_strings string duplication
 by using "MS" section flags
Message-ID: <20191126160209.GC38845@google.com>
References: <20191120145110.8397-1-jeyu@kernel.org>
 <20191125154217.18640-1-jeyu@kernel.org>
 <CAK7LNASU9YysYNXuBKSU4WeUyE=2itfLDYzCupXL-49GUZuGnQ@mail.gmail.com>
 <20191126135620.GA38845@google.com>
 <20191126153153.GA3495@linux-8ccs>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20191126153153.GA3495@linux-8ccs>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 26, 2019 at 04:31:54PM +0100, Jessica Yu wrote:
>+++ Matthias Maennich [26/11/19 13:56 +0000]:
>>On Tue, Nov 26, 2019 at 05:32:59PM +0900, Masahiro Yamada wrote:
>>>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.
>>>>
>>
>>Thanks for working on this!
>>
>>>>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().
>>
>>I agree with this, these entries should be consistent.
>>
>>>
>>>
>>>
>>>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)
>>>
>>
>>I would prefer the separate macros though (as initially proposed) as I
>>find them much more readable. The code is already a bit tricky to reason
>>about and I don't think the shorter version is enough of a gain.
>
>Yeah, the macros were more readable IMO. But I could just squash them into one
>__KSTRTAB_ENTRY macro as a compromise for Masahiro maybe?
>
>Is this any better?

Yes, I like this variant.

>
>diff --git a/include/linux/export.h b/include/linux/export.h
>index 201262793369..f4a8fc798a1b 100644
>--- a/include/linux/export.h
>+++ b/include/linux/export.h
>@@ -81,16 +81,30 @@ 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.
>+ *
>+ * This basically corresponds to:
>+ * const char __kstrtab_##sym[] __attribute__((section("__ksymtab_strings")) = #sym;
>+ * const char __kstrtabns_##sym[] __attribute__((section("__ksymtab_strings")) = ns;
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
You might want to drop the section attribute here to make the comment
shorter (even though it is not entirely correct than anymore).

Cheers,
Matthias

>+ */
>+#define __KSTRTAB_ENTRY(sym, ns)                                               \
>+       asm("   .section \"__ksymtab_strings\",\"aMS\",%progbits,1      \n"     \
>+           "__kstrtab_" #sym ":                                        \n"     \
>+           "   .asciz  \"" #sym "\"                                    \n"     \
>+           "__kstrtabns_" #sym ":                                      \n"     \
>+           "   .asciz  \"" ns "\"                                      \n"     \
>+           "   .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, ns);                                       \
>       __KSYMTAB_ENTRY(sym, sec)
>
>#endif
>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>>+           "   .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;                                                         \
>>
>>You could keep simplified versions of these statements as comment for
>>the above macros to increase readability.
>>
>>>>-       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
>>>>
>>
>>With the above addressed, please feel free to add
>>
>>Reviewed-by: Matthias Maennich <maennich@google.com>
>>
>>Cheers,
>>Matthias
>>
>>>
>>>
>>>-- 
>>>Best Regards
>>>Masahiro Yamada