diff mbox series

[2/4] crypto: qat - Remove zlib-deflate

Message ID E1qbI7x-009Bvo-IM@formenos.hmeau.com
State Accepted
Commit e9dd20e0e5f62d01d9404db2cf9824d1faebcf71
Headers show
Series crypto: Remove zlib-deflate | expand

Commit Message

Herbert Xu Aug. 30, 2023, 10:08 a.m. UTC
Remove the implementation of zlib-deflate because it is completely
unused in the kernel.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 drivers/crypto/intel/qat/qat_common/qat_comp_algs.c |  129 --------------------
 1 file changed, 1 insertion(+), 128 deletions(-)

Comments

Cabiddu, Giovanni Sept. 5, 2023, 1:15 p.m. UTC | #1
Hi Herbert,

On Wed, Aug 30, 2023 at 06:08:47PM +0800, Herbert Xu wrote:
> Remove the implementation of zlib-deflate because it is completely
> unused in the kernel.
We are working at a new revision of [1] which enables BTRFS to use acomp
for offloading zlib-deflate. We see that there is value in using QAT for
such use case in terms of throughput / CPU utilization / compression ratio
compared to software.
Zlib-deflate is preferred to deflate since BTRFS already uses that
format.

We expect to send this patch for 6.7.
Can we keep zlib-deflate in the kernel?

Thanks,

[1] https://patchwork.kernel.org/project/linux-btrfs/patch/1467083180-111750-1-git-send-email-weigang.li@intel.com/
David Sterba Sept. 5, 2023, 4:23 p.m. UTC | #2
On Tue, Sep 05, 2023 at 02:15:44PM +0100, Giovanni Cabiddu wrote:
> Hi Herbert,
> 
> On Wed, Aug 30, 2023 at 06:08:47PM +0800, Herbert Xu wrote:
> > Remove the implementation of zlib-deflate because it is completely
> > unused in the kernel.
> We are working at a new revision of [1] which enables BTRFS to use acomp
> for offloading zlib-deflate. We see that there is value in using QAT for
> such use case in terms of throughput / CPU utilization / compression ratio
> compared to software.
> Zlib-deflate is preferred to deflate since BTRFS already uses that
> format.
> 
> We expect to send this patch for 6.7.
> Can we keep zlib-deflate in the kernel?
> 
> Thanks,
> 
> [1] https://patchwork.kernel.org/project/linux-btrfs/patch/1467083180-111750-1-git-send-email-weigang.li@intel.com/

The patch is from 2016 and zlib though still supported has been
superseded by zstd that is from 2017. It would be good to see numbers
comparing zlib (cpu), zlib (qat) against relevant zstd levels. The
offloading might be an improvement and worth adding the support
otherwise I don't see much reason to add it unless there are users.

I can see there's QAT support for zstd too,
https://github.com/intel/QAT-ZSTD-Plugin, can't find one for lzo but in
case ther's QAT for all 3 algorithms used by btrfs I wouldn't mind
keeping the QAT support for zlib for parity.
Gao Xiang Sept. 6, 2023, 2:51 a.m. UTC | #3
On 2023/9/6 00:23, David Sterba wrote:
> On Tue, Sep 05, 2023 at 02:15:44PM +0100, Giovanni Cabiddu wrote:
>> Hi Herbert,
>>
>> On Wed, Aug 30, 2023 at 06:08:47PM +0800, Herbert Xu wrote:
>>> Remove the implementation of zlib-deflate because it is completely
>>> unused in the kernel.
>> We are working at a new revision of [1] which enables BTRFS to use acomp
>> for offloading zlib-deflate. We see that there is value in using QAT for
>> such use case in terms of throughput / CPU utilization / compression ratio
>> compared to software.
>> Zlib-deflate is preferred to deflate since BTRFS already uses that
>> format.
>>
>> We expect to send this patch for 6.7.
>> Can we keep zlib-deflate in the kernel?
>>
>> Thanks,
>>
>> [1] https://patchwork.kernel.org/project/linux-btrfs/patch/1467083180-111750-1-git-send-email-weigang.li@intel.com/
> 
> The patch is from 2016 and zlib though still supported has been
> superseded by zstd that is from 2017. It would be good to see numbers
> comparing zlib (cpu), zlib (qat) against relevant zstd levels. The
> offloading might be an improvement and worth adding the support
> otherwise I don't see much reason to add it unless there are users.
> 
> I can see there's QAT support for zstd too,
> https://github.com/intel/QAT-ZSTD-Plugin, can't find one for lzo but in
> case ther's QAT for all 3 algorithms used by btrfs I wouldn't mind
> keeping the QAT support for zlib for parity.

Just my personal side note: from my own point of view, QAT actually only
has DEFLATE and LZ4 format hardware end-to-end (de)compression support [1]
(IAA actually has DEFLATE-family format only [2]).

They partially support compressing Zstd format with their internal
hardware lz4s + postprocessing pipeline (mostly a LZ77 matchfinder) by
using hardware (with hw_buffer_sz less than 128KiB. [3][4]) and Zstd
hardware decompression is currently not supported since there is no
such hardware format support.

I'm not saying new Zstd algorithm is not amazing.  Yet from the hardware
perspective, I guess that due to gzip, the original zip, png, pdf, docx,
https, even pppoe all based on DEFLATE (you could see a lot other OSes
support DEFLATE-family format), so it's reasonble to resolve such
de-facto standard with limited hardware first to boost up data center
use cases.  If they have abundant hardware chip room, I guess they will
consider Zstd as well.

As for zlib container format, I don't see zlib Adler-32 checksum is
useful since almost all hardware accelerator supports raw DEFLATE but
Adler-32 checksum is uncommon.  If we consider better integrating
support, we should consider a more common hash (instead Adler-32
checksum) of or by using merkle tree to build the trust chain.

Actually we're working on EROFS raw deflate to enable IAA accelerator
support so we also hope IAA patchset could be landed upstream [5] so
I could upstream my work then.  Therefore, I hope "hisilicon ZIP
driver" could support raw deflate too (after a quick search, their
decompression spend is 1530 MB/s compared with the original zlib
219 MB/s on their platform [6])

[1] https://github.com/intel/QATzip
[2] https://cdrdv2.intel.com/v1/dl/getContent/721858
[3] https://github.com/facebook/zstd/issues/455#issuecomment-1397204587
[4] https://github.com/intel/QATzip/blob/master/utils/qzstd.c#L211
[5] https://lore.kernel.org/r/20230807203726.1682123-1-tom.zanussi@linux.intel.com
[6] https://compare-intel-kunpeng.readthedocs.io/zh_CN/latest/accelerator.html

Thanks,
Gao Xiang
diff mbox series

Patch

diff --git a/drivers/crypto/intel/qat/qat_common/qat_comp_algs.c b/drivers/crypto/intel/qat/qat_common/qat_comp_algs.c
index b533984906ec..bf8c0ee62917 100644
--- a/drivers/crypto/intel/qat/qat_common/qat_comp_algs.c
+++ b/drivers/crypto/intel/qat/qat_common/qat_comp_algs.c
@@ -109,69 +109,6 @@  static void qat_comp_resubmit(struct work_struct *work)
 	acomp_request_complete(areq, ret);
 }
 
-static int parse_zlib_header(u16 zlib_h)
-{
-	int ret = -EINVAL;
-	__be16 header;
-	u8 *header_p;
-	u8 cmf, flg;
-
-	header = cpu_to_be16(zlib_h);
-	header_p = (u8 *)&header;
-
-	flg = header_p[0];
-	cmf = header_p[1];
-
-	if (cmf >> QAT_RFC_1950_CM_OFFSET > QAT_RFC_1950_CM_DEFLATE_CINFO_32K)
-		return ret;
-
-	if ((cmf & QAT_RFC_1950_CM_MASK) != QAT_RFC_1950_CM_DEFLATE)
-		return ret;
-
-	if (flg & QAT_RFC_1950_DICT_MASK)
-		return ret;
-
-	return 0;
-}
-
-static int qat_comp_rfc1950_callback(struct qat_compression_req *qat_req,
-				     void *resp)
-{
-	struct acomp_req *areq = qat_req->acompress_req;
-	enum direction dir = qat_req->dir;
-	__be32 qat_produced_adler;
-
-	qat_produced_adler = cpu_to_be32(qat_comp_get_produced_adler32(resp));
-
-	if (dir == COMPRESSION) {
-		__be16 zlib_header;
-
-		zlib_header = cpu_to_be16(QAT_RFC_1950_COMP_HDR);
-		scatterwalk_map_and_copy(&zlib_header, areq->dst, 0, QAT_RFC_1950_HDR_SIZE, 1);
-		areq->dlen += QAT_RFC_1950_HDR_SIZE;
-
-		scatterwalk_map_and_copy(&qat_produced_adler, areq->dst, areq->dlen,
-					 QAT_RFC_1950_FOOTER_SIZE, 1);
-		areq->dlen += QAT_RFC_1950_FOOTER_SIZE;
-	} else {
-		__be32 decomp_adler;
-		int footer_offset;
-		int consumed;
-
-		consumed = qat_comp_get_consumed_ctr(resp);
-		footer_offset = consumed + QAT_RFC_1950_HDR_SIZE;
-		if (footer_offset + QAT_RFC_1950_FOOTER_SIZE > areq->slen)
-			return -EBADMSG;
-
-		scatterwalk_map_and_copy(&decomp_adler, areq->src, footer_offset,
-					 QAT_RFC_1950_FOOTER_SIZE, 0);
-
-		if (qat_produced_adler != decomp_adler)
-			return -EBADMSG;
-	}
-	return 0;
-}
-
 static void qat_comp_generic_callback(struct qat_compression_req *qat_req,
 				      void *resp)
 {
@@ -293,18 +230,6 @@  static void qat_comp_alg_exit_tfm(struct crypto_acomp *acomp_tfm)
 	memset(ctx, 0, sizeof(*ctx));
 }
 
-static int qat_comp_alg_rfc1950_init_tfm(struct crypto_acomp *acomp_tfm)
-{
-	struct crypto_tfm *tfm = crypto_acomp_tfm(acomp_tfm);
-	struct qat_compression_ctx *ctx = crypto_tfm_ctx(tfm);
-	int ret;
-
-	ret = qat_comp_alg_init_tfm(acomp_tfm);
-	ctx->qat_comp_callback = &qat_comp_rfc1950_callback;
-
-	return ret;
-}
-
 static int qat_comp_alg_compress_decompress(struct acomp_req *areq, enum direction dir,
 					    unsigned int shdr, unsigned int sftr,
 					    unsigned int dhdr, unsigned int dftr)
@@ -400,43 +325,6 @@  static int qat_comp_alg_decompress(struct acomp_req *req)
 	return qat_comp_alg_compress_decompress(req, DECOMPRESSION, 0, 0, 0, 0);
 }
 
-static int qat_comp_alg_rfc1950_compress(struct acomp_req *req)
-{
-	if (!req->dst && req->dlen != 0)
-		return -EINVAL;
-
-	if (req->dst && req->dlen <= QAT_RFC_1950_HDR_SIZE + QAT_RFC_1950_FOOTER_SIZE)
-		return -EINVAL;
-
-	return qat_comp_alg_compress_decompress(req, COMPRESSION, 0, 0,
-						QAT_RFC_1950_HDR_SIZE,
-						QAT_RFC_1950_FOOTER_SIZE);
-}
-
-static int qat_comp_alg_rfc1950_decompress(struct acomp_req *req)
-{
-	struct crypto_acomp *acomp_tfm = crypto_acomp_reqtfm(req);
-	struct crypto_tfm *tfm = crypto_acomp_tfm(acomp_tfm);
-	struct qat_compression_ctx *ctx = crypto_tfm_ctx(tfm);
-	struct adf_accel_dev *accel_dev = ctx->inst->accel_dev;
-	u16 zlib_header;
-	int ret;
-
-	if (req->slen <= QAT_RFC_1950_HDR_SIZE + QAT_RFC_1950_FOOTER_SIZE)
-		return -EBADMSG;
-
-	scatterwalk_map_and_copy(&zlib_header, req->src, 0, QAT_RFC_1950_HDR_SIZE, 0);
-
-	ret = parse_zlib_header(zlib_header);
-	if (ret) {
-		dev_dbg(&GET_DEV(accel_dev), "Error parsing zlib header\n");
-		return ret;
-	}
-
-	return qat_comp_alg_compress_decompress(req, DECOMPRESSION, QAT_RFC_1950_HDR_SIZE,
-						QAT_RFC_1950_FOOTER_SIZE, 0, 0);
-}
-
 static struct acomp_alg qat_acomp[] = { {
 	.base = {
 		.cra_name = "deflate",
@@ -452,22 +340,7 @@  static struct acomp_alg qat_acomp[] = { {
 	.decompress = qat_comp_alg_decompress,
 	.dst_free = sgl_free,
 	.reqsize = sizeof(struct qat_compression_req),
-}, {
-	.base = {
-		.cra_name = "zlib-deflate",
-		.cra_driver_name = "qat_zlib_deflate",
-		.cra_priority = 4001,
-		.cra_flags = CRYPTO_ALG_ASYNC,
-		.cra_ctxsize = sizeof(struct qat_compression_ctx),
-		.cra_module = THIS_MODULE,
-	},
-	.init = qat_comp_alg_rfc1950_init_tfm,
-	.exit = qat_comp_alg_exit_tfm,
-	.compress = qat_comp_alg_rfc1950_compress,
-	.decompress = qat_comp_alg_rfc1950_decompress,
-	.dst_free = sgl_free,
-	.reqsize = sizeof(struct qat_compression_req),
-} };
+}};
 
 int qat_comp_algs_register(void)
 {