| Message ID | 20240508074223.652784-1-senozhatsky@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | zram: convert to custom compression API and allow algorithms tuning | expand |
On Wed, May 08, 2024 at 04:41:53PM +0900, Sergey Senozhatsky wrote: > This patch set moves zram from crypto API to a custom compression > API which allows us to tune and configure compression algorithms, > something that crypto API, unfortunately, doesn't support. [...] > 21 files changed, 1203 insertions(+), 111 deletions(-) Why can't it? This is an awful lot of crazy code duplication just to pass a few parameters.
On (24/05/09 05:43), Christoph Hellwig wrote: > On Wed, May 08, 2024 at 04:41:53PM +0900, Sergey Senozhatsky wrote: > > This patch set moves zram from crypto API to a custom compression > > API which allows us to tune and configure compression algorithms, > > something that crypto API, unfortunately, doesn't support. > > [...] > > > 21 files changed, 1203 insertions(+), 111 deletions(-) > > Why can't it? Well, I asked crypto folks if that's doable and the only reply was "did you try using compression libs directly". And that's not a bad response, I take it. The handling of parameters becomes quite intrusive very quickly. It's not as simple as just passing a new "struct crypto_tfm" to all sort of API abstractions that crypto has, it's a little more than that. Just as an example. For zstd we can work in two modes 1) load the dictionary by_copy 2) load the dictionary by_ref In (2) we need to guarantee that the dictionary memory outlives any comp contexts, so cyrpto_tfm-s now begin to have "external" dependency. But if we load the dictionary by_ref then what we can do is a pre-processing of the dictionary buffer - we get CDict and DDict pointers (specific only to zstd backend) which all contexts now can share (contexts access C/D Dict in read-only mode). For this we need to have a pre-processing stage somewhere in the API and keep the "compression's backend private data" somewhere, then somehow pass it to context cra_init and release that memory when all context were destroyed. In zram I just went with "we do only by_ref" and handle all the dependencies/guarantees, it's very simple because all of this stays in zram. But in general case, a typical crypto API usage tfm = crypto_alloc_comp(comp->name, 0, 0); should become much more complex. I'd say that, probably, developing an entirely new sub-set of API would be simpler. So I implemented a simple zram comp API. I can't tell how much effort it'll be to handle all of this in crypto, I'm not really familiar with crypto, and I'm not sure if crypto API folks are even interested. > This is an awful lot of crazy code duplication just > to pass a few parameters. I see what you mean, but the majority of the code is unique, there isn't too much code duplication in fact. Params handling is unique, dictionary handling is unique, zstd implementation is entirely different and pretty much specific to zram (we don't handle all sort of cases that zstd API support, we focus on things that we need), lz4/lz4hc implementations are also different, etc. etc. Things like lzo/lzorle may count as code duplication, but those are like 20 lines of code or maybe even less (which isn't that crazy).
On Fri, May 10, 2024 at 02:15:09PM +0900, Sergey Senozhatsky wrote: > > Well, I asked crypto folks if that's doable and the only reply was > "did you try using compression libs directly". And that's not a > bad response, I take it. Sorry, I've been busy but I was going to get back to you on this. > But in general case, a typical crypto API usage > > tfm = crypto_alloc_comp(comp->name, 0, 0); > > should become much more complex. I'd say that, probably, developing > an entirely new sub-set of API would be simpler. We could easily add a setparams interface for acomp to support this. The form of parameters would be specific to each individual algorithm (but obviously all drivers for the same algorithm must use the same format). Let me hack something up for you. Cheers,
On (24/05/10 15:40), Herbert Xu wrote: > > But in general case, a typical crypto API usage > > > > tfm = crypto_alloc_comp(comp->name, 0, 0); > > > > should become much more complex. I'd say that, probably, developing > > an entirely new sub-set of API would be simpler. > > We could easily add a setparams interface for acomp to support > this. The form of parameters would be specific to each individual > algorithm (but obviously all drivers for the same algorithm must > use the same format). For some algorithms params needs to be set before ctx is created. For example zstd, crypto/zstd calls zstd_get_params(ZSTD_DEF_LEVEL, 0) to estimate workspace size, which misses the opportunity to configure it an way zram/zswap can benefit from, because those work with PAGE_SIZE source buffer. So for zram zstd_get_params(ZSTD_DEF_LEVEL, PAGE_SIZE) is much better (it saves 1.2MB per ctx, which is per-CPU in zram). Not to mention that zstd_get_params(param->level, 0) is what we need at the end. And then drivers need to be re-implemented to support params. For example, crypto/lz4 should call LZ4_compress_fast() instead of LZ4_compress_default(), because fact() accepts compression level, which is a tunable value.
On Fri, May 10, 2024 at 05:08:27PM +0900, Sergey Senozhatsky wrote: > > For some algorithms params needs to be set before ctx is created. > For example zstd, crypto/zstd calls zstd_get_params(ZSTD_DEF_LEVEL, 0) > to estimate workspace size, which misses the opportunity to configure > it an way zram/zswap can benefit from, because those work with PAGE_SIZE > source buffer. So for zram zstd_get_params(ZSTD_DEF_LEVEL, PAGE_SIZE) > is much better (it saves 1.2MB per ctx, which is per-CPU in zram). Not > to mention that zstd_get_params(param->level, 0) is what we need at the > end. For these algorithms where the overhead of allocating a default set of parameters and then changing them on a setparam call is too high, we could stipulate that the tfm can only be used after a setparam call (just as we require a setkey before cipher ops). Cheers,
On (24/05/10 16:12), Herbert Xu wrote: > On Fri, May 10, 2024 at 05:08:27PM +0900, Sergey Senozhatsky wrote: > > > > For some algorithms params needs to be set before ctx is created. > > For example zstd, crypto/zstd calls zstd_get_params(ZSTD_DEF_LEVEL, 0) > > to estimate workspace size, which misses the opportunity to configure > > it an way zram/zswap can benefit from, because those work with PAGE_SIZE > > source buffer. So for zram zstd_get_params(ZSTD_DEF_LEVEL, PAGE_SIZE) > > is much better (it saves 1.2MB per ctx, which is per-CPU in zram). Not > > to mention that zstd_get_params(param->level, 0) is what we need at the > > end. > > For these algorithms where the overhead of allocating a default > set of parameters and then changing them on a setparam call is > too high, we could stipulate that the tfm can only be used after > a setparam call (just as we require a setkey before cipher ops). OK. I guess for drivers' params support (dictionaries handling etc.) we take take some code from this series. You mentioned acomp, does this mean setparam is for async compression only?
On Fri, May 10, 2024 at 05:28:50PM +0900, Sergey Senozhatsky wrote: > > OK. I guess for drivers' params support (dictionaries handling etc.) > we take take some code from this series. You mentioned acomp, does this > mean setparam is for async compression only? It would be for both acomp and scomp. I have no intention to add it to the legacy comp interface. Cheers,
On (24/05/10 16:30), Herbert Xu wrote: > On Fri, May 10, 2024 at 05:28:50PM +0900, Sergey Senozhatsky wrote: > > > > OK. I guess for drivers' params support (dictionaries handling etc.) > > we take take some code from this series. You mentioned acomp, does this > > mean setparam is for async compression only? > > It would be for both acomp and scomp. I have no intention to > add it to the legacy comp interface. Alright, I'll wait for the patches and then will take a look at how to use them in zram and how I can help with the drivers (if needed).
This patch set moves zram from crypto API to a custom compression API which allows us to tune and configure compression algorithms, something that crypto API, unfortunately, doesn't support. Basically, this seroes brings back the bits of comp "backend" code that we had many years ago. This means that if we want zram to support new compression algorithms we need to implement corresponding backends. Currently, zram supports a pretty decent number of comp backends: lzo, lzorle, lz4, lz4hc, 842, deflate, zstd At this point we handle 2 parameters: a compression level and a pre-trained compression dictionary. Which seems like a good enough start. The list will be extended in the future. Examples: - changes default compression level echo "algo=zstd level=11" > /sys/block/zram0/comp_algorithm - passes path to a pre-trained dictionary echo "algo=zstd dict=/etc/dictionary" > /sys/block/zram0/comp_algorithm Benchmarks ========== *** zstd /sys/block/zram0/mm_stat 1750302720 504600204 514416640 0 514416640 1 0 34204 34204 *** zstd level=5 /sys/block/zram0/mm_stat 1750331392 488449001 497905664 0 497905664 1 0 34204 34204 *** zstd dict=/etc/dictionary /sys/block/zram0/mm_stat 1750335488 464838883 474210304 0 474210304 1 0 34204 34204 *** zstd level=5 dict=/etc/dictionary /sys/block/zram0/mm_stat 1750319104 451907185 461299712 0 461299712 1 0 34204 34204 *** lz4 /sys/block/zram0/mm_stat 1750319104 664253635 676859904 0 676859904 1 0 34288 34288 *** lz4 dict=/etc/dictionary /sys/block/zram0/mm_stat 1750319104 620602911 632705024 0 632705024 1 0 34288 34288 *** lz4hc /sys/block/zram0/mm_stat 1750315008 609004936 621092864 0 621092864 1 0 34288 34288 *** lz4hc level=5 dict=/etc/dictionary /sys/block/zram0/mm_stat 1750323200 501315128 511303680 0 511303680 1 0 34288 34288 v3: -- support pre-trained dictionary in lz4 -- support pre-trained dictionary in lz4hc v2: -- forcibly select LZO when none backeneds were selected (Masahiro Yamada) -- add 842 compression backend -- added comp config preprocessing and caching -- reworked zstd dict handling Sergey Senozhatsky (19): zram: move from crypto API to custom comp backends API zram: add lzo and lzorle compression backends support zram: add lz4 compression backend support zram: add lz4hc compression backend support zram: add zstd compression backend support zram: pass estimated src size hint to zstd zram: add zlib compression backend support zram: add 842 compression backend support zram: check that backends array has at least one backend zram: introduce zcomp_config structure zram: extend comp_algorithm attr write handling zram: support compression level comp config zram: add support for dict comp config zram: add dictionary support to zstd backend zram: add config init/release backend callbacks zram: share dictionaries between per-CPU contexts zram: add dictionary support to lz4 zram: add dictionary support to lz4hc Documentation/zram: add documentation for algorithm parameters Documentation/admin-guide/blockdev/zram.rst | 38 +++- drivers/block/zram/Kconfig | 76 +++++-- drivers/block/zram/Makefile | 8 + drivers/block/zram/backend_842.c | 79 +++++++ drivers/block/zram/backend_842.h | 10 + drivers/block/zram/backend_deflate.c | 143 +++++++++++++ drivers/block/zram/backend_deflate.h | 10 + drivers/block/zram/backend_lz4.c | 123 +++++++++++ drivers/block/zram/backend_lz4.h | 10 + drivers/block/zram/backend_lz4hc.c | 128 ++++++++++++ drivers/block/zram/backend_lz4hc.h | 10 + drivers/block/zram/backend_lzo.c | 55 +++++ drivers/block/zram/backend_lzo.h | 10 + drivers/block/zram/backend_lzorle.c | 55 +++++ drivers/block/zram/backend_lzorle.h | 10 + drivers/block/zram/backend_zstd.c | 221 ++++++++++++++++++++ drivers/block/zram/backend_zstd.h | 10 + drivers/block/zram/zcomp.c | 155 +++++++------- drivers/block/zram/zcomp.h | 42 +++- drivers/block/zram/zram_drv.c | 120 ++++++++++- drivers/block/zram/zram_drv.h | 1 + 21 files changed, 1203 insertions(+), 111 deletions(-) create mode 100644 drivers/block/zram/backend_842.c create mode 100644 drivers/block/zram/backend_842.h create mode 100644 drivers/block/zram/backend_deflate.c create mode 100644 drivers/block/zram/backend_deflate.h create mode 100644 drivers/block/zram/backend_lz4.c create mode 100644 drivers/block/zram/backend_lz4.h create mode 100644 drivers/block/zram/backend_lz4hc.c create mode 100644 drivers/block/zram/backend_lz4hc.h create mode 100644 drivers/block/zram/backend_lzo.c create mode 100644 drivers/block/zram/backend_lzo.h create mode 100644 drivers/block/zram/backend_lzorle.c create mode 100644 drivers/block/zram/backend_lzorle.h create mode 100644 drivers/block/zram/backend_zstd.c create mode 100644 drivers/block/zram/backend_zstd.h