From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 115221B427 for ; Wed, 11 Jul 2018 15:25:49 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Jul 2018 06:25:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,338,1526367600"; d="scan'208";a="55713261" Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by orsmga007.jf.intel.com with ESMTP; 11 Jul 2018 06:25:47 -0700 Received: from irsmsx107.ger.corp.intel.com ([169.254.10.193]) by IRSMSX103.ger.corp.intel.com ([169.254.3.208]) with mapi id 14.03.0319.002; Wed, 11 Jul 2018 14:25:46 +0100 From: "De Lara Guarch, Pablo" To: Shally Verma CC: "dev@dpdk.org" , "pathreya@caviumnetworks.com" , "mchalla@caviumnetworks.com" , Sunila Sahu , Sunila Sahu , Ashish Gupta Thread-Topic: [PATCH v2 4/5] compress/zlib: add enq deq apis Thread-Index: AQHUEiXZBKBV8Mz4306icjmAYbh8W6SKAQow Date: Wed, 11 Jul 2018 13:25:45 +0000 Message-ID: References: <1530550631-22841-1-git-send-email-shally.verma@caviumnetworks.com> <1530550631-22841-5-git-send-email-shally.verma@caviumnetworks.com> In-Reply-To: <1530550631-22841-5-git-send-email-shally.verma@caviumnetworks.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjNkNDFiZGItOTI5Yi00OWVlLWI0YzgtY2Q0MDczYzZhYmE3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiUDRXMlwveFFhQ1J3NFJRdkxibE9obFZYcFwvZHNyaGlqVjJNcTNBRE9ab0VqV0FzZFwvdm9kbXlCVE1CWFU5VVNKViJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 4/5] 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: Wed, 11 Jul 2018 13:25:50 -0000 Hi, > -----Original Message----- > From: Shally Verma [mailto:shally.verma@caviumnetworks.com] > Sent: Monday, July 2, 2018 5:57 PM > To: De Lara Guarch, Pablo > Cc: dev@dpdk.org; pathreya@caviumnetworks.com; > mchalla@caviumnetworks.com; Sunila Sahu ; > Sunila Sahu ; Ashish Gupta > > Subject: [PATCH v2 4/5] compress/zlib: add enq deq apis Better retitle to "support burst enqueue/dequeue". >=20 > From: Sunila Sahu >=20 > implement enqueue and dequeue apis This should start with capital letter, but this is probably not needed anyw= ay. >=20 > Signed-off-by: Sunila Sahu > Signed-off-by: Shally Verma > Signed-off-by: Ashish Gupta > --- > drivers/compress/zlib/zlib_pmd.c | 238 > ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 237 insertions(+), 1 deletion(-) >=20 > diff --git a/drivers/compress/zlib/zlib_pmd.c b/drivers/compress/zlib/zli= b_pmd.c > index 7c2614e..aef23de 100644 > --- a/drivers/compress/zlib/zlib_pmd.c > +++ b/drivers/compress/zlib/zlib_pmd.c > @@ -6,7 +6,198 @@ > #include > #include "zlib_pmd_private.h" >=20 > -/** Parse comp xform and set private xform/stream parameters */ > +/** Compute next mbuf in the list, assign data buffer and len */ > +#define COMPUTE_BUF(mbuf, data, len) \ > + ((mbuf =3D mbuf->next) ? \ > + (data =3D rte_pktmbuf_mtod(mbuf, uint8_t *)), \ > + (len =3D rte_pktmbuf_data_len(mbuf)) : 0) > + > +/** Set op->status to appropriate flag if we run out of mbuf */ > +#define COMPUTE_DST_BUF(mbuf, dst, dlen) \ > + ((COMPUTE_BUF(mbuf, dst, dlen)) ? 1 : \ > + (!(op->status =3D \ I see and build error here: drivers/compress/zlib/zlib_pmd.c(81): error #187: use of "=3D" where "=3D= =3D" may have been intended COMPUTE_DST_BUF(mbuf_dst, strm->next_out, It is a bit confusing macro, but afaik, you should pass op if you want to m= odify the status (I suggest making this more readable). =20 > + RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED))) > + > +static void > +process_zlib_deflate(struct rte_comp_op *op, z_stream *strm) { > + int ret, flush, fin_flush; > + struct rte_mbuf *mbuf_src =3D op->m_src; > + struct rte_mbuf *mbuf_dst =3D op->m_dst; > + > + switch (op->flush_flag) { > + case RTE_COMP_FLUSH_FULL: > + case RTE_COMP_FLUSH_FINAL: > + fin_flush =3D Z_FINISH; > + break; > + default: > + op->status =3D RTE_COMP_OP_STATUS_INVALID_ARGS; > + ZLIB_PMD_ERR("\nInvalid flush value"); Better to have "\n" at the end for consistency. > + > + } > + > + if (unlikely(!strm)) { > + op->status =3D RTE_COMP_OP_STATUS_INVALID_ARGS; > + ZLIB_PMD_ERR("\nInvalid z_stream"); > + return; > + } > + /* Update z_stream with the inputs provided by application */ > + strm->next_in =3D rte_pktmbuf_mtod_offset(mbuf_src, uint8_t *, > + op->src.offset); > + > + strm->avail_in =3D rte_pktmbuf_data_len(mbuf_src) - op->src.offset; Shouldn't this be "op->src.length"? > + > + strm->next_out =3D rte_pktmbuf_mtod_offset(mbuf_dst, uint8_t *, > + op->dst.offset); > + > + strm->avail_out =3D rte_pktmbuf_data_len(mbuf_dst) - op->dst.offset; > + > + /* Set flush value to NO_FLUSH unless it is last mbuf */ > + flush =3D Z_NO_FLUSH; > + /* Initialize status to SUCCESS */ > + op->status =3D RTE_COMP_OP_STATUS_SUCCESS; > + ... > + /* Update source buffer to next mbuf > + * Exit if op->status is not SUCCESS or input buffers are fully consume= d > + */ > + } while ((op->status =3D=3D RTE_COMP_OP_STATUS_SUCCESS) && > + (COMPUTE_BUF(mbuf_src, strm->next_in, strm->avail_in))); It looks like you support Scatter Gather here, but according to the documen= tation, you don't support it... > + > +def_end: > + /* Update op stats */ > + switch (op->status) { > + case RTE_COMP_OP_STATUS_SUCCESS: > + op->consumed +=3D strm->total_in; I see a compilation error with gcc: drivers/compress/zlib/zlib_pmd.c:166:16: error: this statement may fall through [-Werror=3Dimplicit-fallthrough=3D] op->consumed +=3D strm->total_in; ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~ drivers/compress/zlib/zlib_pmd.c:167:2: note: here case RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED: ^~~~ If the intention is to fall-through, you should add a comment before the ne= xt case. > + case RTE_COMP_OP_STATUS_OUT_OF_SPACE_TERMINATED: > + op->produced +=3D strm->total_out; > + break; > + default: > + ZLIB_PMD_ERR("stats not updated for status:%d\n", > + op->status); > + } > + > + deflateReset(strm); > +} > + ... > + > +/** Process comp operation for mbuf */ > +static inline int > +process_zlib_op(struct zlib_qp *qp, struct rte_comp_op *op) { > + struct zlib_stream *stream; > + > + if ((op->op_type =3D=3D RTE_COMP_OP_STATEFUL) || > + op->src.offset > rte_pktmbuf_data_len(op->m_src) || > + op->dst.offset > rte_pktmbuf_data_len(op->m_dst)) { An extra indentation is needed, so it is easy to distinguish between the if= statement and the body. You should also check if (src.offset + src.length > rte_pktmbuf_data_len(op= ->m_src)) > + op->status =3D RTE_COMP_OP_STATUS_INVALID_ARGS; > + ZLIB_PMD_ERR("\nInvalid source or destination buffers or " > + "invalid Operation requested"); Better to include the "\n" at the end. > + } else { > + stream =3D &((struct zlib_priv_xform *)op->private_xform)- > >stream; I think it is more readable to have this line split into two: First get the private xform and then get zlib_stream pointer. > + stream->comp(op, &stream->strm); > + } > + /* whatever is out of op, put it into completion queue with > + * its status > + */ > + return rte_ring_enqueue(qp->processed_pkts, (void *)op); } > + ... > +static uint16_t > +zlib_pmd_enqueue_burst(void *queue_pair, > + struct rte_comp_op **ops, uint16_t nb_ops) { > + struct zlib_qp *qp =3D queue_pair; > + int ret, i; > + int enqd =3D 0; "i" and "enqd" variables should be uint16_t. > + for (i =3D 0; i < nb_ops; i++) { > + ret =3D process_zlib_op(qp, ops[i]); I think you should check if there is enough space in the ring for all the o= perations, to save time. If there is not, then the PMD would modify the operation unnecessarily and = waste a lot of time. Then, with that check, you can just process the operations that can fit, lo= oping until minimum between nb_ops and space available in the ring. > + if (unlikely(ret < 0)) { > + /* increment count if failed to push to completion > + * queue > + */ > + qp->qp_stats.enqueue_err_count++; I think this variable should be used when there was an error in the operati= on but it was still be enqueued (because there was space in the ring). So you can increase this count in process_zlib_op when there is an error. > + } else { > + qp->qp_stats.enqueued_count++; I think this should be equal to all the operations enqueued (with and witho= ut error). > + enqd++; > + } > + } > + return enqd; > +}