Comments inline.
> -----Original Message-----
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Shally Verma
> Sent: Tuesday, May 15, 2018 11:32 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com>
> Cc: Trahe, Fiona <fiona.tr...@intel.com>; dev@dpdk.org;
> pathr...@caviumnetworks.com; Sunila Sahu
> <sunila.s...@caviumnetworks.com>; Ashish Gupta
> <ashish.gu...@caviumnetworks.com>
> Subject: [dpdk-dev] [PATCH v1 4/6] compress/zlib: add enq deq apis
>
> implement enqueue and dequeue apis
<...>
> diff --git a/drivers/compress/zlib/meson.build
> b/drivers/compress/zlib/meson.build
> new file mode 100644
> index 0000000..d66de95
> --- /dev/null
> +++ b/drivers/compress/zlib/meson.build
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2018 Cavium
> +Networks
> +
> +dep = dependency('zlib', required: false) if not dep.found()
> + build = false
> +endif
> +deps += 'bus_vdev'
> +sources = files('zlib_pmd.c', 'zlib_pmd_ops.c') ext_deps += dep
> +pkgconfig_extra_libs += '-lz'
[Lee] You have added "allow_experimental_apis" tag to the zlib/Makefile, better
to add to the meson as well.
With the tag; allow_experimental_apis = true.
> diff --git a/drivers/compress/zlib/zlib_pmd.c
> b/drivers/compress/zlib/zlib_pmd.c
> index 3dc71ec..e2681a7 100644
> --- a/drivers/compress/zlib/zlib_pmd.c
> +++ b/drivers/compress/zlib/zlib_pmd.c
> @@ -17,7 +17,239 @@
> #include "zlib_pmd_private.h"
>
> static uint8_t compressdev_driver_id;
> -int zlib_logtype_driver;
> +
> +/** compute the next mbuf in list and assign dst buffer and dlen,
> + * set op->status to appropriate flag if we run out of mbuf */
> +#define COMPUTE_DST_BUF(mbuf, dst, dlen) \
> + ((mbuf = mbuf->next) ?
> \
> + (dst = rte_pktmbuf_mtod(mbuf, uint8_t *)), \
> + dlen = rte_pktmbuf_data_len(mbuf) : \
> + !(op->status = \
> + ((op->op_type == RTE_COMP_OP_STATELESS) ?
> \
[Lee] Clever & useful macro.
> +
> RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED : \
> +
> RTE_COMP_OP_STATUS_OUT_OF_SPACE_RECOVERABLE)))
> +
> +static void
> +process_zlib_deflate(struct rte_comp_op *op, z_stream *strm) {
> + int ret, flush, fin_flush;
> + uint8_t *src, *dst;
> + uint32_t sl, dl, have;
> + struct rte_mbuf *mbuf_src = op->m_src;
> + struct rte_mbuf *mbuf_dst = op->m_dst;
> +
> + src = rte_pktmbuf_mtod_offset(mbuf_src, uint8_t *, op-
> >src.offset);
> +
> + sl = rte_pktmbuf_data_len(mbuf_src) - op->src.offset;
> +
> + dst = rte_pktmbuf_mtod_offset(mbuf_dst, unsigned char *,
> + op->dst.offset);
> +
> + dl = rte_pktmbuf_data_len(mbuf_dst) - op->dst.offset;
> +
> + if (unlikely(!src || !dst || !strm)) {
> + op->status = RTE_COMP_OP_STATUS_INVALID_ARGS;
> + ZLIB_LOG_ERR("\nInvalid source or destination buffers");
> + return;
> + }
> + switch (op->flush_flag) {
> + case RTE_COMP_FLUSH_NONE:
> + fin_flush = Z_NO_FLUSH;
> + break;
> + case RTE_COMP_FLUSH_SYNC:
> + fin_flush = Z_SYNC_FLUSH;
> + break;
> + case RTE_COMP_FLUSH_FULL:
> + fin_flush = Z_FULL_FLUSH;
> + break;
> + case RTE_COMP_FLUSH_FINAL:
> + fin_flush = Z_FINISH;
> + break;
> + default:
> + op->status = RTE_COMP_OP_STATUS_ERROR;
> + goto def_end;
> + }
> + if (op->src.length <= sl) {
> + sl = op->src.length;
> + flush = fin_flush;
> + } else {
> + /* if there're more than one mbufs in input,
> + * process intermediate with NO_FLUSH
> + */
> + flush = Z_NO_FLUSH;
> + }
> + /* initialize status to SUCCESS */
> + op->status = RTE_COMP_OP_STATUS_SUCCESS;
> +
> + do {
> + /* Update z_stream with the inputs provided by application
> */
> + strm->next_in = src;
> + strm->avail_in = sl;
> +
> + do {
> + strm->avail_out = dl;
> + strm->next_out = dst;
> + ret = deflate(strm, flush);
> + if (unlikely(ret == Z_STREAM_ERROR)) {
> + /* error return, do not process further */
> + op->status =
> RTE_COMP_OP_STATUS_ERROR;
> + goto def_end;
> + }
> + /* Update op stats */
> + op->produced += dl - strm->avail_out;
> + op->consumed += sl - strm->avail_in;
[Lee] strm struct has a total_in & total_out field which can be used here and
will save these cycles doing the calculation each iteration.
> + /* Break if Z_STREAM_END is encountered or dst mbuf gets
> over */
> + } while (!(ret == Z_STREAM_END) && (strm->avail_out == 0)
> &&
> + COMPUTE_DST_BUF(mbuf_dst, dst, dl));
> +
> + /** Compress till the end of compressed blocks provided
> + * or till Z_FINISH
> + * Exit if op->status is not SUCCESS.
> + */
> + if ((op->status != RTE_COMP_OP_STATUS_SUCCESS) ||
> + (ret == Z_STREAM_END) ||
> + op->consumed == op->src.length)
> + goto def_end;
> +
> + /** Update last output buffer with respect to availed space
> */
> + have = dl - strm->avail_out;
> + dst += have;
> + dl = strm->avail_out;
[Lee] From what I can see this assignment isn't doing anything, in the while
condition macro dl gets reassigned and inside the above while loop
"strm->avail_out = dl", but I may be missing something.
> + /** Update source buffer to next mbuf*/
> + mbuf_src = mbuf_src->next;
> + src = rte_pktmbuf_mtod(mbuf_src, uint8_t *);
> + sl = rte_pktmbuf_data_len(mbuf_src);
> +
> + /** Last block to be compressed
> + * Update flush with value provided by app for last block,
> + * For stateless flush should be always Z_FINISH
> + */
> +
> + if ((op->src.length - op->consumed) <= sl) {
> + sl = (op->src.length - op->consumed);
> + flush = fin_flush;
[Lee] Just a clarification of my understanding please, this assignment of
fin_flush to flush is what will cause the last return from the zlib deflate to
be Z_STREAM_END, only if fin_flush is set to Z_FINISH. fin_flush is passed from
the application, if the application doesn't pass Z_FINISH as the flush flag (I
See your comment above.), this means there will never be a Z_STREAM_END return
from deflate, and eventually deflate will just return an error. I believe a
check could be done long before this point to ensure the flush flag is Z_FINAL,
since the PMD only supports stateful at the moment. This will save cycles
compressing data that we only find out at the end of the op is not valid.
> + }
> +
> + } while (1);
[Lee] This will be to be rewritten as to not use an infinite while loop.
Undefined behaviour may lead to system hanging, so we don't use them in DPDK.
> +def_end:
> + if (op->op_type == RTE_COMP_OP_STATELESS)
> + deflateReset(strm);
> +}
> +
> +static void
> +process_zlib_inflate(struct rte_comp_op *op, z_stream *strm) {
> + int ret, flush;
> + uint8_t *src, *dst;
> + uint32_t sl, dl, have;
> + struct rte_mbuf *mbuf_src = op->m_src;
> + struct rte_mbuf *mbuf_dst = op->m_dst;
> +
[Lee] same comments apply for inflate as deflate.
Thanks,
Lee.
