| Message ID | E1qbI7x-009Bvo-IM@formenos.hmeau.com |
|---|---|
| State | Accepted |
| Commit | e9dd20e0e5f62d01d9404db2cf9824d1faebcf71 |
| Headers | show |
| Series | crypto: Remove zlib-deflate | expand |
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/
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.
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 --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) {
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(-)