Received-SPF: pass (google.com: domain of linux-crypto+bounces-4084-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 10 May 2024 14:15:09 +0900
From: Sergey Senozhatsky <senozhatsky@chromium.org>
To: Christoph Hellwig <hch@infradead.org>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Minchan Kim <minchan@kernel.org>, linux-kernel@vger.kernel.org,
	linux-block@vger.kernel.org,
	Herbert Xu <herbert@gondor.apana.org.au>,
	"David S. Miller" <davem@davemloft.net>,
	linux-crypto@vger.kernel.org
Subject: Re: [PATCHv3 00/19] zram: convert to custom compression API and
 allow algorithms tuning
Message-ID: <20240510051509.GI8623@google.com>
References: <20240508074223.652784-1-senozhatsky@chromium.org>
 <ZjzFB2CzCh1NKlfw@infradead.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZjzFB2CzCh1NKlfw@infradead.org>

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