DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Daly, Lee" <lee.daly@intel.com>
To: Shally Verma <shally.verma@caviumnetworks.com>
Cc: "Trahe, Fiona" <fiona.trahe@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"pathreay@caviumnetworks.com" <pathreay@caviumnetworks.com>,
	Sunila Sahu <sunila.sahu@caviumnetworks.com>,
	Ashish Gupta <ashish.gupta@caviumnetworks.com>,
	"De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>
Subject: Re: [dpdk-dev] [PATCH v1 4/6] compress/zlib: add enq deq apis
Date: Fri, 15 Jun 2018 11:09:13 +0000	[thread overview]
Message-ID: <F5C6929789601049BEB7272E267355985749A7@IRSMSX106.ger.corp.intel.com> (raw)
In-Reply-To: <1526380346-7386-5-git-send-email-shally.verma@caviumnetworks.com>

Comments inline.

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shally Verma
> Sent: Tuesday, May 15, 2018 11:32 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org;
> pathreay@caviumnetworks.com; Sunila Sahu
> <sunila.sahu@caviumnetworks.com>; Ashish Gupta
> <ashish.gupta@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.

  reply	other threads:[~2018-06-15 11:09 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-15 10:32 [dpdk-dev] [PATCH v1 0/6]compress: add zlib compression PMD Shally Verma
2018-05-15 10:32 ` [dpdk-dev] [PATCH v1 1/6] compress/zlib: add ZLIB PMD support Shally Verma
2018-06-15 11:08   ` Daly, Lee
2018-05-15 10:32 ` [dpdk-dev] [PATCH v1 2/6] compress/zlib: add device setup PMD ops Shally Verma
2018-06-15 11:08   ` Daly, Lee
2018-06-22 13:21     ` Verma, Shally
2018-06-25 10:05       ` Daly, Lee
2018-06-25 10:07         ` Verma, Shally
2018-05-15 10:32 ` [dpdk-dev] [PATCH v1 3/6] compress/zlib: add xform and stream create support Shally Verma
2018-06-15 11:09   ` Daly, Lee
2018-05-15 10:32 ` [dpdk-dev] [PATCH v1 4/6] compress/zlib: add enq deq apis Shally Verma
2018-06-15 11:09   ` Daly, Lee [this message]
2018-05-15 10:32 ` [dpdk-dev] [PATCH v1 5/6] test: add ZLIB PMD for compressdev tests Shally Verma
2018-05-15 10:32 ` [dpdk-dev] [PATCH v1 6/6] doc: add ZLIB PMD documentation Shally Verma
2018-06-15 11:09   ` Daly, Lee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=F5C6929789601049BEB7272E267355985749A7@IRSMSX106.ger.corp.intel.com \
    --to=lee.daly@intel.com \
    --cc=ashish.gupta@caviumnetworks.com \
    --cc=dev@dpdk.org \
    --cc=fiona.trahe@intel.com \
    --cc=pablo.de.lara.guarch@intel.com \
    --cc=pathreay@caviumnetworks.com \
    --cc=shally.verma@caviumnetworks.com \
    --cc=sunila.sahu@caviumnetworks.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).