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:   Wed, 20 Nov 2019 16:02:16 +0100
From:   Jessica Yu <jeyu@kernel.org>
To:     linux-kernel@vger.kernel.org
Cc:     Matthias Maennich <maennich@google.com>,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH] export.h: reduce __ksymtab_strings string duplication by
 using "MS" section flags
Message-ID: <20191120150215.GA756@linux-8ccs>
References: <20191120145110.8397-1-jeyu@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20191120145110.8397-1-jeyu@kernel.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

+++ Jessica Yu [20/11/19 15:51 +0100]:
>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>
>---
> include/asm-generic/export.h | 13 ++++++++++---
> include/linux/export.h       | 28 ++++++++++++++++++++++------
> 2 files changed, 32 insertions(+), 9 deletions(-)
>
>diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
>index fa577978fbbd..d0704f2602f4 100644
>--- a/include/asm-generic/export.h
>+++ b/include/asm-generic/export.h
>@@ -26,9 +26,16 @@
> .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 vs %progbits since usage of
>+ * "M" (SHF_MERGE) section flag requires it.
>  */
>+
>+#ifdef CONFIG_ARM
>+#define ARCH_PROGBITS %progbits
>+#else
>+#define ARCH_PROGBITS @progbits
>+#endif
>+
> .macro ___EXPORT_SYMBOL name,val,sec
> #ifdef CONFIG_MODULES
> 	.globl __ksymtab_\name
>@@ -37,7 +44,7 @@
> __ksymtab_\name:
> 	__put \val, __kstrtab_\name
> 	.previous
>-	.section __ksymtab_strings,"a"
>+	.section __ksymtab_strings,"aMS",ARCH_PROGBITS,1
> __kstrtab_\name:
> 	.asciz "\name"
> 	.previous
>diff --git a/include/linux/export.h b/include/linux/export.h
>index 201262793369..ab325a8e6bee 100644
>--- a/include/linux/export.h
>+++ b/include/linux/export.h
>@@ -81,16 +81,32 @@ struct kernel_symbol {
>
> #else
>
>+#ifdef CONFIG_ARM
>+#define ARCH_PROGBITS "%progbits"
>+#else
>+#define ARCH_PROGBITS "@progbits"
>+#endif

Just a comment, I don't like the duplication of this block, but I
wasn't sure where was the best place to have this define since
asm-generic/export.h and linux/export.h don't share includes, and
includes are generally kept to a minimum. If anyone has a better
idea, please let me know.