Hi,
Okay, here is a very lightly tested version of using zstd directly, which
should save 128KB per CPU compare to using crypto API. This leaves the
crypto API available, though, if someone wants to use it instead. Even
supporting both, this is a net reduction in code, due to dropping all
the zbufsize logic.
How does this look?
-Kees
Kees Cook (5):
pstore: Remove worse-case compression size logic
pstore: Allow for arbitrary compression algorithm
pstore: Use size_t for compress/decompression type widths
pstore: Refactor compression initialization
pstore: Use zstd directly by default for compression
fs/pstore/Kconfig | 131 +++++-----------
fs/pstore/platform.c | 358 ++++++++++++++++++++-----------------------
2 files changed, 209 insertions(+), 280 deletions(-)
--
2.34.1
The worst case compression size is always the size of the uncompressed
data itself so avoid perfectly optimizing the oops buffer size. Hugely
simplifies the code.
Cc: Tony Luck <[email protected]>
Cc: "Guilherme G. Piccoli" <[email protected]>
Cc: Nick Terrell <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
fs/pstore/platform.c | 144 +++----------------------------------------
1 file changed, 9 insertions(+), 135 deletions(-)
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index cbc0b468c1ab..ef0bc3ae161b 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -97,11 +97,6 @@ MODULE_PARM_DESC(kmsg_bytes, "amount of kernel log to snapshot (in bytes)");
/* Compression parameters */
static struct crypto_comp *tfm;
-struct pstore_zbackend {
- int (*zbufsize)(size_t size);
- const char *name;
-};
-
static char *big_oops_buf;
static size_t big_oops_buf_sz;
@@ -168,105 +163,6 @@ static bool pstore_cannot_block_path(enum kmsg_dump_reason reason)
}
}
-#if IS_ENABLED(CONFIG_PSTORE_DEFLATE_COMPRESS)
-static int zbufsize_deflate(size_t size)
-{
- size_t cmpr;
-
- switch (size) {
- /* buffer range for efivars */
- case 1000 ... 2000:
- cmpr = 56;
- break;
- case 2001 ... 3000:
- cmpr = 54;
- break;
- case 3001 ... 3999:
- cmpr = 52;
- break;
- /* buffer range for nvram, erst */
- case 4000 ... 10000:
- cmpr = 45;
- break;
- default:
- cmpr = 60;
- break;
- }
-
- return (size * 100) / cmpr;
-}
-#endif
-
-#if IS_ENABLED(CONFIG_PSTORE_LZO_COMPRESS)
-static int zbufsize_lzo(size_t size)
-{
- return lzo1x_worst_compress(size);
-}
-#endif
-
-#if IS_ENABLED(CONFIG_PSTORE_LZ4_COMPRESS) || IS_ENABLED(CONFIG_PSTORE_LZ4HC_COMPRESS)
-static int zbufsize_lz4(size_t size)
-{
- return LZ4_compressBound(size);
-}
-#endif
-
-#if IS_ENABLED(CONFIG_PSTORE_842_COMPRESS)
-static int zbufsize_842(size_t size)
-{
- return size;
-}
-#endif
-
-#if IS_ENABLED(CONFIG_PSTORE_ZSTD_COMPRESS)
-static int zbufsize_zstd(size_t size)
-{
- return zstd_compress_bound(size);
-}
-#endif
-
-static const struct pstore_zbackend *zbackend __ro_after_init;
-
-static const struct pstore_zbackend zbackends[] = {
-#if IS_ENABLED(CONFIG_PSTORE_DEFLATE_COMPRESS)
- {
- .zbufsize = zbufsize_deflate,
- .name = "deflate",
- },
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_LZO_COMPRESS)
- {
- .zbufsize = zbufsize_lzo,
- .name = "lzo",
- },
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_LZ4_COMPRESS)
- {
- .zbufsize = zbufsize_lz4,
- .name = "lz4",
- },
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_LZ4HC_COMPRESS)
- {
- .zbufsize = zbufsize_lz4,
- .name = "lz4hc",
- },
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_842_COMPRESS)
- {
- .zbufsize = zbufsize_842,
- .name = "842",
- },
-#endif
-#if IS_ENABLED(CONFIG_PSTORE_ZSTD_COMPRESS)
- {
- .zbufsize = zbufsize_zstd,
- .name = "zstd",
- },
-#endif
- { }
-};
-
static int pstore_compress(const void *in, void *out,
unsigned int inlen, unsigned int outlen)
{
@@ -291,36 +187,31 @@ static void allocate_buf_for_compression(void)
char *buf;
/* Skip if not built-in or compression backend not selected yet. */
- if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend)
+ if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !compress)
return;
/* Skip if no pstore backend yet or compression init already done. */
if (!psinfo || tfm)
return;
- if (!crypto_has_comp(zbackend->name, 0, 0)) {
- pr_err("Unknown compression: %s\n", zbackend->name);
- return;
- }
-
- size = zbackend->zbufsize(psinfo->bufsize);
- if (size <= 0) {
- pr_err("Invalid compression size for %s: %d\n",
- zbackend->name, size);
+ if (!crypto_has_comp(compress, 0, 0)) {
+ pr_err("Unknown compression: %s\n", compress);
return;
}
+ /* Worst-case compression should never be more than uncompressed. */
+ size = psinfo->bufsize;
buf = kmalloc(size, GFP_KERNEL);
if (!buf) {
pr_err("Failed %d byte compression buffer allocation for: %s\n",
- size, zbackend->name);
+ size, compress);
return;
}
- ctx = crypto_alloc_comp(zbackend->name, 0, 0);
+ ctx = crypto_alloc_comp(compress, 0, 0);
if (IS_ERR_OR_NULL(ctx)) {
kfree(buf);
- pr_err("crypto_alloc_comp('%s') failed: %ld\n", zbackend->name,
+ pr_err("crypto_alloc_comp('%s') failed: %ld\n", compress,
PTR_ERR(ctx));
return;
}
@@ -330,7 +221,7 @@ static void allocate_buf_for_compression(void)
big_oops_buf_sz = size;
big_oops_buf = buf;
- pr_info("Using crash dump compression: %s\n", zbackend->name);
+ pr_info("Using crash dump compression: %s\n", compress);
}
static void free_buf_for_compression(void)
@@ -818,27 +709,10 @@ static void pstore_timefunc(struct timer_list *unused)
pstore_timer_kick();
}
-static void __init pstore_choose_compression(void)
-{
- const struct pstore_zbackend *step;
-
- if (!compress)
- return;
-
- for (step = zbackends; step->name; step++) {
- if (!strcmp(compress, step->name)) {
- zbackend = step;
- return;
- }
- }
-}
-
static int __init pstore_init(void)
{
int ret;
- pstore_choose_compression();
-
/*
* Check if any pstore backends registered earlier but did not
* initialize compression because crypto was not ready. If so,
--
2.34.1
On Tue, 18 Oct 2022 at 04:08, Kees Cook <[email protected]> wrote:
>
> Hi,
>
> Okay, here is a very lightly tested version of using zstd directly, which
> should save 128KB per CPU compare to using crypto API. This leaves the
> crypto API available, though, if someone wants to use it instead. Even
> supporting both, this is a net reduction in code, due to dropping all
> the zbufsize logic.
>
> How does this look?
>
The code changes all look correct and reasonable to me.
As for supporting both the library and the crypto API interface: I did
a little digging, and it turns out all additional compression modes
were added by the same contributor, with no justification other than
'this might be useful to some people' (see below)
However, I did a quick experiment with the output of dmesg on my
workstation, and these are the results I am getting
Input file:
-rw-r--r-- 1 ard ard 111792 Oct 18 09:23 uncompressed
Default compression
-rw-r--r-- 1 ard ard 21810 Oct 18 09:23 uncompressed.gz
-rw-r--r-- 1 ard ard 33923 Oct 18 09:23 uncompressed.lz4
-rw-r--r-- 1 ard ard 34238 Oct 18 09:23 uncompressed.lzo
-rw-r--r-- 1 ard ard 21599 Oct 18 09:23 uncompressed.zst
Max compression (-9)
-rw-r--r-- 1 ard ard 21589 Oct 18 09:23 uncompressed.gz
-rw-r--r-- 1 ard ard 28848 Oct 18 09:25 uncompressed.lz4 (== lz4hc?)
-rw-r--r-- 1 ard ard 26917 Oct 18 09:23 uncompressed.lzo
-rw-r--r-- 1 ard ard 19883 Oct 18 09:23 uncompressed.zst
So the patches in question don't actually fulfill their claim of
improving the compression ratio. Only zstd, which was added later,
improves upon zlib, but not significantly unless you override the
compression level (which we don't).
So I seriously doubt that those patches were inspired by the need to
solve an actual problem anyone was experiencing at the time, given
that they don't. It just seems that nobody bothered to ask the 'why?'
question.
So again, I suggest to simply drop this non-feature, and standardize
on either zlib or zstd using the library interface exclusively. If
someone present a compelling use case, we can always consider adding
it back in some form.
As for the choice of algorithm, given the equal performance using the
default compression level, and the difference in code size, I don't
see why zstd should be preferred here. If anything, it only increases
the likelihood of hitting another error if we are panicking due to
some memory corruption issue.
$ size lib/zstd/zstd_compress.ko lib/zlib_deflate/zlib_deflate.ko
text data bss dec hex filename
218411 768 0 219179 3582b lib/zstd/zstd_compress.ko
16231 876 2288 19395 4bc3 lib/zlib_deflate/zlib_deflate.ko
commit 8cfc8ddc99df9509a46043b14af81f5c6a223eab
Author: Geliang Tang <[email protected]>
AuthorDate: Thu Feb 18 22:04:22 2016 +0800
Commit: Kees Cook <[email protected]>
CommitDate: Thu Jun 2 10:59:31 2016 -0700
pstore: add lzo/lz4 compression support
Like zlib compression in pstore, this patch added lzo and lz4
compression support so that users can have more options and better
compression ratio.
commit 239b716199d9aff0d09444b0086e23aacd6bd445
Author: Geliang Tang <[email protected]>
AuthorDate: Tue Feb 13 14:40:39 2018 +0800
Commit: Kees Cook <[email protected]>
CommitDate: Tue Mar 6 15:06:11 2018 -0800
pstore: Add lz4hc and 842 compression support
>
> Kees Cook (5):
> pstore: Remove worse-case compression size logic
> pstore: Allow for arbitrary compression algorithm
> pstore: Use size_t for compress/decompression type widths
> pstore: Refactor compression initialization
> pstore: Use zstd directly by default for compression
>
> fs/pstore/Kconfig | 131 +++++-----------
> fs/pstore/platform.c | 358 ++++++++++++++++++++-----------------------
> 2 files changed, 209 insertions(+), 280 deletions(-)
>
> --
> 2.34.1
>
On 18/10/2022 05:20, Ard Biesheuvel wrote:
> [...]
> So again, I suggest to simply drop this non-feature, and standardize
> on either zlib or zstd using the library interface exclusively. If
> someone present a compelling use case, we can always consider adding
> it back in some form.
>
> As for the choice of algorithm, given the equal performance using the
> default compression level, and the difference in code size, I don't
> see why zstd should be preferred here. If anything, it only increases
> the likelihood of hitting another error if we are panicking due to
> some memory corruption issue.
I think it's a good argument - would zlib be simpler in code than zstd?
I've checked the zstd patch from Kees - not complex per se, but would be
great if we could have a simple mechanism, without the need of the ifdef
there for example...
Cheers,
Guilherme
On Tue, 18 Oct 2022 at 16:11, Guilherme G. Piccoli <[email protected]> wrote:
>
> On 18/10/2022 05:20, Ard Biesheuvel wrote:
> > [...]
> > So again, I suggest to simply drop this non-feature, and standardize
> > on either zlib or zstd using the library interface exclusively. If
> > someone present a compelling use case, we can always consider adding
> > it back in some form.
> >
> > As for the choice of algorithm, given the equal performance using the
> > default compression level, and the difference in code size, I don't
> > see why zstd should be preferred here. If anything, it only increases
> > the likelihood of hitting another error if we are panicking due to
> > some memory corruption issue.
>
> I think it's a good argument - would zlib be simpler in code than zstd?
I think it should be rather straight-forward. Note that this is what
we had before 2016 when all the 'features' were starting to get added.
> I've checked the zstd patch from Kees - not complex per se, but would be
> great if we could have a simple mechanism, without the need of the ifdef
> there for example...
>
> Cheers,
>
>
> Guilherme