From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 5737414E8 for ; Fri, 13 Jul 2018 17:56:01 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Jul 2018 08:56:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,348,1526367600"; d="scan'208";a="54816132" Received: from irsmsx110.ger.corp.intel.com ([163.33.3.25]) by fmsmga008.fm.intel.com with ESMTP; 13 Jul 2018 08:55:59 -0700 Received: from irsmsx107.ger.corp.intel.com ([169.254.10.193]) by irsmsx110.ger.corp.intel.com ([169.254.15.197]) with mapi id 14.03.0319.002; Fri, 13 Jul 2018 16:55:58 +0100 From: "De Lara Guarch, Pablo" To: "Verma, Shally" CC: "dev@dpdk.org" , "Athreya, Narayana Prasad" , "Challa, Mahipal" , "Sahu, Sunila" , "Sahu, Sunila" , "Gupta, Ashish" Thread-Topic: [PATCH v2 4/5] compress/zlib: add enq deq apis Thread-Index: AQHUEiXZBKBV8Mz4306icjmAYbh8W6SKAQowgAEROACAAkNUwA== Date: Fri, 13 Jul 2018 15:55:57 +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: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZWIyMDg3MjYtMDYxMS00NTVkLWI4YzUtMDQ3ZWRiNWYwYTVlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiSlB0bmJ4Z09iYnJndGFINFwvcStXNjh2Rk9pVXRDdEVIWnhMVytyV2pGZ2tucGkxOVRzYXRTaWZYVnRmbUxRYkwifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [163.33.239.181] 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: Fri, 13 Jul 2018 15:56:03 -0000 Hi Shally, Sorry for the delay. Comments inline. > -----Original Message----- > From: Verma, Shally [mailto:Shally.Verma@cavium.com] > Sent: Thursday, July 12, 2018 6:46 AM > To: De Lara Guarch, Pablo > Cc: dev@dpdk.org; Athreya, Narayana Prasad > ; Challa, Mahipal > ; Sahu, Sunila ; Sahu, > Sunila ; Gupta, Ashish > Subject: RE: [PATCH v2 4/5] compress/zlib: add enq deq apis >=20 >=20 >=20 > >-----Original Message----- > >From: De Lara Guarch, Pablo [mailto:pablo.de.lara.guarch@intel.com] > >Sent: 11 July 2018 18:56 > >To: Verma, Shally > >Cc: dev@dpdk.org; Athreya, Narayana Prasad > >; Challa, Mahipal > >; Sahu, Sunila ; > >Sahu, Sunila ; Gupta, Ashish > > > >Subject: RE: [PATCH v2 4/5] compress/zlib: add enq deq apis > > > >External Email > > > >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". > > > >> > >> From: Sunila Sahu > >> > >> implement enqueue and dequeue apis > > > >This should start with capital letter, but this is probably not needed a= nyway. > > > >> > >> 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(-) > >> > >> diff --git a/drivers/compress/zlib/zlib_pmd.c > >> b/drivers/compress/zlib/zlib_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" > >> > >> -/** 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 > >modify the status (I suggest making this more readable). > > > > > >> + 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"? > If packet is segmented, then src.length will give wrong o/p. Though we ar= e not > claiming segmented packet support. But this is safer way to do. Right, I was assuming that only single segment buffers were supported. Regardless you take into account single or multi segment buffers, src.length needs to be taken into consideration. >=20 > > > >> + > >> + 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 = consumed > >> + */ > >> + } 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 > documentation, you don't support it... > > > Yes. right now, we don't claim its support as we couldn't test it. Tests for this were sent in this release: http://patches.dpdk.org/patch/42137/ If you think you support it, you should try the tests and set the flags. >=20 > >> + > >> +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= next > case. > > > Ok. But then my view is it shouldn't be taken as error here. Is it we don= 't want > fall-through model? Basically the compiler warns you that you could have forgotten a break stat= ement. If you want to fall-though, you should add "/* Fall-through */ before the n= ext case. >=20 > >> + 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)) >=20 > Should it be checked against pktmbuf_data_len() or pktmbuf_pkt_len()?! Pktlen if you consider multi-segment (which I thought you weren't supportin= g it). For a single segment, these two values are the same. >=20 > > > >> + 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 th= e > operations, to save time. > >If there is not, then the PMD would modify the operation unnecessarily a= nd > waste a lot of time. > >Then, with that check, you can just process the operations that can > >fit, looping until minimum between nb_ops and space available in the rin= g. > I doubt if I should do that. If there's an application with dequeue only = thread, > then at one point, we may see it full and another space available. Since = this PMD > is using lockless rings, it can be made flexible to produce as many it ca= n. Right, I see your point. My concern is that if there is no space in the rin= g, you would have modified the operation. The expectation is that, if an opera= tion cannot be enqueued, you can retry and enqueue it again, but if you have modified it, that won't= be possible. An alternative is to move the processing into the dequeue function. >=20 > > > >> + if (unlikely(ret < 0)) { > >> + /* increment count if failed to push to completi= on > >> + * queue > >> + */ > >> + qp->qp_stats.enqueue_err_count++; > > > >I think this variable should be used when there was an error in the > >operation but it was still be enqueued (because there was space in the r= ing). > >So you can increase this count in process_zlib_op when there is an error= . > > > So what is exact purpose of this stat? is enqueue_err_count supposed to = give > number of failed operations in processed queue? > Or, just number of operations that couldn't be enqueued/dequeued for > processing? For me, it is the number of operations that were enqueued, but that contain= s an error. This actually raises a concern. For software devices, operations with error= s can be enqueued and not processed, but for hardware devices, this might not work (arguments could be invalid) = and then operation couldn't be "enqueued". In that case, I believe that the faulty operations and the next ones won't = be enqueued into the device, making it inconsistent with software devices. Any opinions on this? >=20 >=20 > >> + } else { > >> + qp->qp_stats.enqueued_count++; > > > >I think this should be equal to all the operations enqueued (with and wi= thout > error). > Yes. Right now it does that except for the cases, where op couldn't be pu= shed > into as processing queue, and user cannot deque it. >=20 > Thanks > Shally > > > >> + enqd++; > >> + } > >> + } > >> + return enqd; > >> +}