From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id B0F291D723 for ; Fri, 15 Jun 2018 13:09:17 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Jun 2018 04:09:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,226,1526367600"; d="scan'208";a="208342233" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by orsmga004.jf.intel.com with ESMTP; 15 Jun 2018 04:09:15 -0700 Received: from irsmsx111.ger.corp.intel.com (10.108.20.4) by IRSMSX151.ger.corp.intel.com (163.33.192.59) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 15 Jun 2018 12:09:14 +0100 Received: from irsmsx106.ger.corp.intel.com ([169.254.8.80]) by irsmsx111.ger.corp.intel.com ([169.254.2.230]) with mapi id 14.03.0319.002; Fri, 15 Jun 2018 12:09:14 +0100 From: "Daly, Lee" To: Shally Verma CC: "Trahe, Fiona" , "dev@dpdk.org" , "pathreay@caviumnetworks.com" , Sunila Sahu , Ashish Gupta , "De Lara Guarch, Pablo" Thread-Topic: [dpdk-dev] [PATCH v1 4/6] compress/zlib: add enq deq apis Thread-Index: AQHT7Dg89P0VJ3eSzEGG2qfIW7T8FaRhNCjA Date: Fri, 15 Jun 2018 11:09:13 +0000 Message-ID: References: <1526380346-7386-1-git-send-email-shally.verma@caviumnetworks.com> <1526380346-7386-5-git-send-email-shally.verma@caviumnetworks.com> In-Reply-To: <1526380346-7386-5-git-send-email-shally.verma@caviumnetworks.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjNkNzgwNDUtYWJhOC00NWM5LThmYmItMDI4ZWU3YTY0ZTlhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJtRnlLallcLzhYVVVseDQyOFgyMFVOMHdJclcyY2EycGdsN2NXbkFCUHVoNGU0V0M0eGhVMFRQN0tnS1dsMytzOSJ9 x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v1 4/6] compress/zlib: add enq deq apis X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Jun 2018 11:09:18 -0000 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 > Cc: Trahe, Fiona ; dev@dpdk.org; > pathreay@caviumnetworks.com; Sunila Sahu > ; Ashish Gupta > > Subject: [dpdk-dev] [PATCH v1 4/6] compress/zlib: add enq deq apis >=20 > 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 =3D dependency('zlib', required: false) if not dep.found() > + build =3D false > +endif > +deps +=3D 'bus_vdev' > +sources =3D files('zlib_pmd.c', 'zlib_pmd_ops.c') ext_deps +=3D dep > +pkgconfig_extra_libs +=3D '-lz' [Lee] You have added "allow_experimental_apis" tag to the zlib/Makefile, be= tter to add to the meson as well. With the tag; allow_experimental_apis =3D 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" >=20 > 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 =3D mbuf->next) ? > \ > + (dst =3D rte_pktmbuf_mtod(mbuf, uint8_t *)), \ > + dlen =3D rte_pktmbuf_data_len(mbuf) : \ > + !(op->status =3D \ > + ((op->op_type =3D=3D 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 =3D op->m_src; > + struct rte_mbuf *mbuf_dst =3D op->m_dst; > + > + src =3D rte_pktmbuf_mtod_offset(mbuf_src, uint8_t *, op- > >src.offset); > + > + sl =3D rte_pktmbuf_data_len(mbuf_src) - op->src.offset; > + > + dst =3D rte_pktmbuf_mtod_offset(mbuf_dst, unsigned char *, > + op->dst.offset); > + > + dl =3D rte_pktmbuf_data_len(mbuf_dst) - op->dst.offset; > + > + if (unlikely(!src || !dst || !strm)) { > + op->status =3D 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 =3D Z_NO_FLUSH; > + break; > + case RTE_COMP_FLUSH_SYNC: > + fin_flush =3D Z_SYNC_FLUSH; > + break; > + case RTE_COMP_FLUSH_FULL: > + fin_flush =3D Z_FULL_FLUSH; > + break; > + case RTE_COMP_FLUSH_FINAL: > + fin_flush =3D Z_FINISH; > + break; > + default: > + op->status =3D RTE_COMP_OP_STATUS_ERROR; > + goto def_end; > + } > + if (op->src.length <=3D sl) { > + sl =3D op->src.length; > + flush =3D fin_flush; > + } else { > + /* if there're more than one mbufs in input, > + * process intermediate with NO_FLUSH > + */ > + flush =3D Z_NO_FLUSH; > + } > + /* initialize status to SUCCESS */ > + op->status =3D RTE_COMP_OP_STATUS_SUCCESS; > + > + do { > + /* Update z_stream with the inputs provided by application > */ > + strm->next_in =3D src; > + strm->avail_in =3D sl; > + > + do { > + strm->avail_out =3D dl; > + strm->next_out =3D dst; > + ret =3D deflate(strm, flush); > + if (unlikely(ret =3D=3D Z_STREAM_ERROR)) { > + /* error return, do not process further */ > + op->status =3D > RTE_COMP_OP_STATUS_ERROR; > + goto def_end; > + } > + /* Update op stats */ > + op->produced +=3D dl - strm->avail_out; > + op->consumed +=3D sl - strm->avail_in; [Lee] strm struct has a total_in & total_out field which can be used here a= nd will save these cycles doing the calculation each iteration. > + /* Break if Z_STREAM_END is encountered or dst mbuf gets > over */ > + } while (!(ret =3D=3D Z_STREAM_END) && (strm->avail_out =3D=3D 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 !=3D RTE_COMP_OP_STATUS_SUCCESS) || > + (ret =3D=3D Z_STREAM_END) || > + op->consumed =3D=3D op->src.length) > + goto def_end; > + > + /** Update last output buffer with respect to availed space > */ > + have =3D dl - strm->avail_out; > + dst +=3D have; > + dl =3D strm->avail_out; [Lee] From what I can see this assignment isn't doing anything, in the whil= e condition macro dl gets reassigned and inside the above while loop "strm-= >avail_out =3D dl", but I may be missing something. =20 > + /** Update source buffer to next mbuf*/ > + mbuf_src =3D mbuf_src->next; > + src =3D rte_pktmbuf_mtod(mbuf_src, uint8_t *); > + sl =3D 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) <=3D sl) { > + sl =3D (op->src.length - op->consumed); > + flush =3D fin_flush; [Lee] Just a clarification of my understanding please, this assignment of f= in_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 pass= ed from the application, if the application doesn't pass Z_FINISH as the fl= ush flag (I See your comment above.), this means there will never be a Z_ST= REAM_END return from deflate, and eventually deflate will just return an er= ror. I believe a check could be done long before this point to ensure the f= lush flag is Z_FINAL, since the PMD only supports stateful at the moment. T= his will save cycles compressing data that we only find out at the end of t= he op is not valid. > + } > + > + } while (1); [Lee] This will be to be rewritten as to not use an infinite while loop. Un= defined behaviour may lead to system hanging, so we don't use them in DPDK. > +def_end: > + if (op->op_type =3D=3D 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 =3D op->m_src; > + struct rte_mbuf *mbuf_dst =3D op->m_dst; > + [Lee] same comments apply for inflate as deflate. Thanks, Lee.