lkml.org 
[lkml]   [2019]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2] export.h: reduce __ksymtab_strings string duplication by using "MS" section flags
+++ 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?

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;
+ */
+#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

\
 
 \ /
  Last update: 2019-11-26 16:32    [W:0.104 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site