From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 2274D2C31 for ; Thu, 29 Dec 2016 16:58:04 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga105.jf.intel.com with ESMTP; 29 Dec 2016 07:58:03 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,427,1477983600"; d="scan'208";a="1105681411" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by fmsmga002.fm.intel.com with ESMTP; 29 Dec 2016 07:58:02 -0800 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.230]) by IRSMSX101.ger.corp.intel.com ([169.254.1.112]) with mapi id 14.03.0248.002; Thu, 29 Dec 2016 15:58:02 +0000 From: "Kulasek, TomaszX" To: Olivier Matz CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 1/4] rte_mbuf: add rte_pktmbuf_coalesce Thread-Index: AQHSTL64vDrDJ/gBOkGsTv69aJS5tKEKbocAgBTKjsA= Date: Thu, 29 Dec 2016 15:58:01 +0000 Message-ID: <3042915272161B4EB253DA4D77EB373A14F6F79A@IRSMSX102.ger.corp.intel.com> References: <1480698466-17620-1-git-send-email-tomaszx.kulasek@intel.com> <1480698466-17620-2-git-send-email-tomaszx.kulasek@intel.com> <20161216110604.7097488e@platinum> In-Reply-To: <20161216110604.7097488e@platinum> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: 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 1/4] rte_mbuf: add rte_pktmbuf_coalesce 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: Thu, 29 Dec 2016 15:58:05 -0000 Hi Olivier, > -----Original Message----- > From: Olivier Matz [mailto:olivier.matz@6wind.com] > Sent: Friday, December 16, 2016 11:06 > To: Kulasek, TomaszX > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 1/4] rte_mbuf: add rte_pktmbuf_coalesce >=20 > Hi Tomasz, >=20 > On Fri, 2 Dec 2016 18:07:43 +0100, Tomasz Kulasek > wrote: > > This patch adds function rte_pktmbuf_coalesce to let crypto PMD > > coalesce chained mbuf before crypto operation and extend their > > capabilities to support segmented mbufs when device cannot handle > > them natively. > > > > > > Signed-off-by: Tomasz Kulasek > > --- > > lib/librte_mbuf/rte_mbuf.h | 34 ++++++++++++++++++++++++++++++++++ > > 1 file changed, 34 insertions(+) > > > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > > index ead7c6e..f048681 100644 > > --- a/lib/librte_mbuf/rte_mbuf.h > > +++ b/lib/librte_mbuf/rte_mbuf.h > > @@ -1647,6 +1647,40 @@ static inline int rte_pktmbuf_chain(struct > > rte_mbuf *head, struct rte_mbuf *tail } > > > > /** > > + * Coalesce data from mbuf to the continuous buffer. > > + * > > + * @param mbuf_dst > > + * Contiguous destination mbuf > > + * @param mbuf_src > > + * Uncontiguous source mbuf > > + * > > + * @return > > + * - 0, on success > > + * - -EINVAL, on error > > + */ >=20 > I think the API should be clarified. In your case, it is expected that th= e > destination mbuf is already filled with uninitialized data (i.e. that > rte_pktmbuf_append() has been called). >=20 > We could wonder if a better API wouldn't be to allocate the dst mbuf in > the function, call append()/prepend(), and do the copy. >=20 > Even better, we could have: >=20 > int rte_pktmbuf_linearize(struct rte_mbuf *m) >=20 > It will reuse the same mbuf (maybe moving the data). >=20 >=20 > > + > > +#include >=20 > This should be removed. >=20 > > + > > +static inline int > > +rte_pktmbuf_coalesce(struct rte_mbuf *mbuf_dst, struct rte_mbuf > *mbuf_src) { >=20 > Source mbuf should be const. >=20 > > + char *dst; > > + > > + if (!rte_pktmbuf_is_contiguous(mbuf_dst) || > > + rte_pktmbuf_data_len(mbuf_dst) >=3D > > + rte_pktmbuf_pkt_len(mbuf_src)) > > + return -EINVAL; >=20 > Why >=3D ? >=20 > > + > > + dst =3D rte_pktmbuf_mtod(mbuf_dst, char *); > > + > > + if (!__rte_pktmbuf_read(mbuf_src, 0, rte_pktmbuf_pkt_len(mbuf_src), > > + dst)) >=20 > When a function returns a pointer, I think it is clearer to do: > if (func() =3D=3D NULL) > than: > if (!func()) >=20 >=20 > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > +/** > > * Dump an mbuf structure to a file. > > * > > * Dump all fields for the given packet mbuf and all its associated >=20 >=20 > One more question, I don't see where this function is used in your > patchset. What is your use-case? >=20 > Regards, > Olivier This function is needed for crypto-perf application: http://dpdk.org/dev/pa= tchwork/patch/17492/ to compare performance of crypto operations on segment= ed mbufs, when scatter gather is or is not supported by crypto PMD. It will= be introduced with v2. When device doesn't support scatter-gather, we want to know an overhead of = manual coalescing mbuf. struct rte_cryptodev_info dev_info; int linearize =3D 0; /* Check if source mbufs require coalescing */ if (ctx->options->segments_nb > 1) { rte_cryptodev_info_get(ctx->dev_id, &dev_info); if ((dev_info.feature_flags & RTE_CRYPTODEV_FF_MBUF_SCATTER_GATHER) =3D=3D 0) linearize =3D 1; } // ... =20 if (linearize) { /* PMD doesn't support scatter-gather and source buffer * is segmented. * We need to linearize it before enqueuing. */ for (i =3D 0; i < burst_size; i++) rte_pktmbuf_linearize(ops[i]->sym->m_src); } /* Enqueue burst of ops on crypto device */ ops_enqd =3D rte_cryptodev_enqueue_burst(ctx->dev_id, ctx->qp_id, ops, burst_size); I have checked this use case (in crypto-perf application), and you're right= , using rte_pktmbuf_linearize() function here is better and more straightfo= rward. I will change it in v2. Thanks for suggestions Tomasz.