Message-ID: <20191205164252.GD148072@google.com>
Date:   Thu, 5 Dec 2019 16:42:52 +0000
From:   Matthias Maennich <maennich@...gle.com>
To:     Jessica Yu <jeyu@...nel.org>
Cc:     Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v2] export.h: reduce __ksymtab_strings string duplication
 by using "MS" section flags

On Tue, Nov 26, 2019 at 05:48:40PM +0100, Jessica Yu wrote:
>+++ Masahiro Yamada [27/11/19 01:12 +0900]:
>>On Wed, Nov 27, 2019 at 12:32 AM Jessica Yu <jeyu@...nel.org> 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@...nel.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@...musvillemoes.dk>
>>>>>>Signed-off-by: Jessica Yu <jeyu@...nel.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 '@...gbits' 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 '@...gbits' 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?
>>
>>I prefer opposite.
>>
>>
>>__CRC_SYMBOL() is macrofied because it is ifdef'ed by
>>CONFIG_MODVERSIONS and CONFIG_MOD_REL_CRCS.
>>
>>__KSYMTAB_ENTRY() is macrofied because it is ifdef'ed by
>>CONFIG_HAVE_ARCH_PREL32_RELOCATIONS.
>>
>>
>>
>>__KSTRTAB_ENTRY() does not depend on any CONFIG option,
>>so it can be expanded in ___EXPORT_SYMBOL().
>>
>>
>>You need to check multiple locations
>>to understand how it works as a whole.
>>I do not understand why increasing macro-chains is readable.
>
>I think it is "readable" in the sense that when someone is reading
>export.h for the first time, they know exactly what ___EXPORT_SYMBOL()
>is supposed to do, without requiring them to read too deeply into the
>asm and swim through the #ifdefs.  __CRC_SYMBOL(), depending on
>modversions, creates an entry in what would eventually be merged to
>become the __kcrctab section, __KSTRTAB_ENTRY() creates entries in
>__ksymtab_strings, and __KSYMTAB_ENTRY() creates an entry in (what
>would eventually become) the __ksymtab{,_gpl} section. It gives a
>general idea of what it's doing.

I agree with that thought. One option would be, though, to put a proper
comment there to explain what happens in the code. Splitting the
functionality across several macros has the effect that the code feels
easier to reason about and has some implicit documentation through the
macro names. If we could restore that in Masahiro's version, I am ok
with that.

Cheers,
Matthias

>
>However, I do understand your reasoning behind not having an extra
>macro. I'll wait a bit before respinning the patch to see if we can
>get at a consensus..
>
>

Powered by blists - more mailing lists