Messages in this thread | | | Date | Tue, 26 Nov 2019 16:02:09 +0000 | From | Matthias Maennich <> | Subject | Re: [PATCH v2] export.h: reduce __ksymtab_strings string duplication by using "MS" section flags |
| |
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
| |