Messages in this thread | | | Date | Wed, 19 Aug 2020 11:14:34 -0700 | From | Kees Cook <> | Subject | Re: [PATCH 1/1] x86/boot/compressed: Use builtin mem functions for decompressor |
| |
On Tue, Aug 04, 2020 at 07:48:17PM -0400, Arvind Sankar wrote: > Since commits > > c041b5ad8640 ("x86, boot: Create a separate string.h file to provide standard string functions") > fb4cac573ef6 ("x86, boot: Move memcmp() into string.h and string.c") > > the decompressor stub has been using the compiler's builtin memcpy, > memset and memcmp functions, _except_ where it would likely have the > largest impact, in the decompression code itself. > > Remove the #undef's of memcpy and memset in misc.c so that the > decompressor code also uses the compiler builtins. > > The rationale given in the comment doesn't really apply: just because > some functions use the out-of-line version is no reason to not use the > builtin version in the rest. > > Replace the comment with an explanation of why memzero and memmove are > being #define'd. > > Drop the suggestion to #undef in boot/string.h as well: the out-of-line > versions are not really optimized versions, they're generic code that's > good enough for the preboot environment. The compiler will likely > generate better code for constant-size memcpy/memset/memcmp if it is > allowed to. > > Most decompressors' performance is unchanged, with the exception of LZ4 > and 64-bit ZSTD. > > Before After ARCH > LZ4 73ms 10ms 32 > LZ4 120ms 10ms 64 > ZSTD 90ms 74ms 64 > > Measurements on QEMU on 2.2GHz Broadwell Xeon, using defconfig kernels. > > Decompressor code size has small differences, with the largest being > that 64-bit ZSTD decreases just over 2k. The largest code size increase > was on 64-bit XZ, of about 400 bytes. > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> > Suggested-by: Nick Terrell <nickrterrell@gmail.com> > Tested-by: Nick Terrell <nickrterrell@gmail.com>
Did anyone pick this up? (Ingo can you snag it, or maybe akpm who took the LZ4-specific patch already?) This looks sane to me and provides some surprising performance benefits. :)
Acked-by: Kees Cook <keescook@chromium.org>
-Kees
> --- > arch/x86/boot/compressed/misc.c | 7 ++----- > arch/x86/boot/string.h | 5 +---- > 2 files changed, 3 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c > index 39e592d0e0b4..e478e40fbe5a 100644 > --- a/arch/x86/boot/compressed/misc.c > +++ b/arch/x86/boot/compressed/misc.c > @@ -30,12 +30,9 @@ > #define STATIC static > > /* > - * Use normal definitions of mem*() from string.c. There are already > - * included header files which expect a definition of memset() and by > - * the time we define memset macro, it is too late. > + * Provide definitions of memzero and memmove as some of the decompressors will > + * try to define their own functions if these are not defined as macros. > */ > -#undef memcpy > -#undef memset > #define memzero(s, n) memset((s), 0, (n)) > #define memmove memmove > > diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h > index 995f7b7ad512..a232da487cd2 100644 > --- a/arch/x86/boot/string.h > +++ b/arch/x86/boot/string.h > @@ -11,10 +11,7 @@ void *memcpy(void *dst, const void *src, size_t len); > void *memset(void *dst, int c, size_t len); > int memcmp(const void *s1, const void *s2, size_t len); > > -/* > - * Access builtin version by default. If one needs to use optimized version, > - * do "undef memcpy" in .c file and link against right string.c > - */ > +/* Access builtin version by default. */ > #define memcpy(d,s,l) __builtin_memcpy(d,s,l) > #define memset(d,c,l) __builtin_memset(d,c,l) > #define memcmp __builtin_memcmp > -- > 2.26.2 >
-- Kees Cook
| |