| Message ID | 20200325195849.407900-2-nickrterrell@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for ZSTD-compressed kernel and initramfs | expand |
Hi Nick, I finally got some time to review your patch, here are my comments: On Wed, Mar 25, 2020 at 12:58:42PM -0700, Nick Terrell wrote: > * Don't export symbols if ZSTD_PREBOOT is defined. I'm not sure if this is needed. When I worked on my patch, I have found that all exporting and modinfo macros generate symbols in modinfo and discard.ksym sections, which are then dropped by the vmlinux linker script, thus one will get the same binary independently if he puts this change in or not. I'm not sure if this is intentional as there is also __DISABLE_EXPORTS define, which should be used by a decompressor (according to comments in export.h). > * Remove a double definition of the CHECK_F macro when the zstd > library is amalgamated. > * Switch ZSTD_copy8() to __builtin_memcpy(), because in the preboot > environment on x86 gcc can't inline `memcpy()` otherwise. > * Limit the gcc hack in ZSTD_wildcopy() to the broken gcc version. See > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81388. No comments to the rest. Petr
> On Mar 26, 2020, at 8:40 AM, Petr Malat <oss@malat.biz> wrote: > > Hi Nick, > I finally got some time to review your patch, here are my comments: > > On Wed, Mar 25, 2020 at 12:58:42PM -0700, Nick Terrell wrote: >> * Don't export symbols if ZSTD_PREBOOT is defined. > I'm not sure if this is needed. When I worked on my patch, I have found that > all exporting and modinfo macros generate symbols in modinfo and discard.ksym > sections, which are then dropped by the vmlinux linker script, thus one > will get the same binary independently if he puts this change in or not. > > I'm not sure if this is intentional as there is also __DISABLE_EXPORTS define, > which should be used by a decompressor (according to comments in export.h). This is not my area of expertise, I’m a zstd developer not a kernel developer. For that reason I wanted to pick the safe route of disabling the exports explicitly, since that is what the other decompressors do, so I know it works. If you’re confident that it isn’t necessary, I can drop the modification. But, I do prefer this approach, because there is no magic involved. I can see what is happening clearly just by looking at the decompress_unzstd.c code. Thanks for the review, Nick
diff --git a/lib/zstd/decompress.c b/lib/zstd/decompress.c index 269ee9a796c1..73ded63278cf 100644 --- a/lib/zstd/decompress.c +++ b/lib/zstd/decompress.c @@ -2490,6 +2490,7 @@ size_t ZSTD_decompressStream(ZSTD_DStream *zds, ZSTD_outBuffer *output, ZSTD_inB } } +#ifndef ZSTD_PREBOOT EXPORT_SYMBOL(ZSTD_DCtxWorkspaceBound); EXPORT_SYMBOL(ZSTD_initDCtx); EXPORT_SYMBOL(ZSTD_decompressDCtx); @@ -2529,3 +2530,4 @@ EXPORT_SYMBOL(ZSTD_insertBlock); MODULE_LICENSE("Dual BSD/GPL"); MODULE_DESCRIPTION("Zstd Decompressor"); +#endif diff --git a/lib/zstd/fse_decompress.c b/lib/zstd/fse_decompress.c index a84300e5a013..0b353530fb3f 100644 --- a/lib/zstd/fse_decompress.c +++ b/lib/zstd/fse_decompress.c @@ -47,6 +47,7 @@ ****************************************************************/ #include "bitstream.h" #include "fse.h" +#include "zstd_internal.h" #include <linux/compiler.h> #include <linux/kernel.h> #include <linux/string.h> /* memcpy, memset */ @@ -60,14 +61,6 @@ enum { FSE_static_assert = 1 / (int)(!!(c)) }; \ } /* use only *after* variable declarations */ -/* check and forward error code */ -#define CHECK_F(f) \ - { \ - size_t const e = f; \ - if (FSE_isError(e)) \ - return e; \ - } - /* ************************************************************** * Templates ****************************************************************/ diff --git a/lib/zstd/zstd_internal.h b/lib/zstd/zstd_internal.h index 1a79fab9e13a..dac753397f86 100644 --- a/lib/zstd/zstd_internal.h +++ b/lib/zstd/zstd_internal.h @@ -127,7 +127,14 @@ static const U32 OF_defaultNormLog = OF_DEFAULTNORMLOG; * Shared functions to include for inlining *********************************************/ ZSTD_STATIC void ZSTD_copy8(void *dst, const void *src) { - memcpy(dst, src, 8); + /* + * zstd relies heavily on gcc being able to analyze and inline this + * memcpy() call, since it is called in a tight loop. Preboot mode + * is compiled in freestanding mode, which stops gcc from analyzing + * memcpy(). Use __builtin_memcpy() to tell gcc to analyze this as a + * regular memcpy(). + */ + __builtin_memcpy(dst, src, 8); } /*! ZSTD_wildcopy() : * custom version of memcpy(), can copy up to 7 bytes too many (8 bytes if length==0) */ @@ -137,13 +144,16 @@ ZSTD_STATIC void ZSTD_wildcopy(void *dst, const void *src, ptrdiff_t length) const BYTE* ip = (const BYTE*)src; BYTE* op = (BYTE*)dst; BYTE* const oend = op + length; - /* Work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81388. +#if defined(GCC_VERSION) && GCC_VERSION >= 70000 && GCC_VERSION < 70200 + /* + * Work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81388. * Avoid the bad case where the loop only runs once by handling the * special case separately. This doesn't trigger the bug because it * doesn't involve pointer/integer overflow. */ if (length <= 8) return ZSTD_copy8(dst, src); +#endif do { ZSTD_copy8(op, ip); op += 8;