Re: [PATCHv3 00/19] zram: convert to custom compression API and allow algorithms tuning

Re: [PATCHv3 00/19] zram: convert to custom compression API and allow algorithms tuning

[Christoph Hellwig]

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.

Re: [PATCHv3 00/19] zram: convert to custom compression API and allow algorithms tuning

[Sergey Senozhatsky]

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).

Re: [PATCHv3 00/19] zram: convert to custom compression API and allow algorithms tuning

[Herbert Xu]

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCHv3 00/19] zram: convert to custom compression API and allow algorithms tuning

[Sergey Senozhatsky]

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.

Re: [PATCHv3 00/19] zram: convert to custom compression API and allow algorithms tuning

[Herbert Xu]

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCHv3 00/19] zram: convert to custom compression API and allow algorithms tuning

[Sergey Senozhatsky]

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?

Re: [PATCHv3 00/19] zram: convert to custom compression API and allow algorithms tuning

[Herbert Xu]

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCHv3 00/19] zram: convert to custom compression API and allow algorithms tuning

[Sergey Senozhatsky]

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).