Hi Olivier, > -----Original Message----- > From: Olivier Matz [mailto:olivier.m...@6wind.com] > Sent: Friday, December 16, 2016 11:06 > To: Kulasek, TomaszX <tomaszx.kula...@intel.com> > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 1/4] rte_mbuf: add rte_pktmbuf_coalesce > > Hi Tomasz, > > On Fri, 2 Dec 2016 18:07:43 +0100, Tomasz Kulasek > <tomaszx.kula...@intel.com> 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 <tomaszx.kula...@intel.com> > > --- > > 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 > > + */ > > I think the API should be clarified. In your case, it is expected that the > destination mbuf is already filled with uninitialized data (i.e. that > rte_pktmbuf_append() has been called). > > 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. > > Even better, we could have: > > int rte_pktmbuf_linearize(struct rte_mbuf *m) > > It will reuse the same mbuf (maybe moving the data). > > > > + > > +#include <rte_hexdump.h> > > This should be removed. > > > + > > +static inline int > > +rte_pktmbuf_coalesce(struct rte_mbuf *mbuf_dst, struct rte_mbuf > *mbuf_src) { > > Source mbuf should be const. > > > + char *dst; > > + > > + if (!rte_pktmbuf_is_contiguous(mbuf_dst) || > > + rte_pktmbuf_data_len(mbuf_dst) >= > > + rte_pktmbuf_pkt_len(mbuf_src)) > > + return -EINVAL; > > Why >= ? > > > + > > + dst = rte_pktmbuf_mtod(mbuf_dst, char *); > > + > > + if (!__rte_pktmbuf_read(mbuf_src, 0, rte_pktmbuf_pkt_len(mbuf_src), > > + dst)) > > When a function returns a pointer, I think it is clearer to do: > if (func() == NULL) > than: > if (!func()) > > > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > +/** > > * Dump an mbuf structure to a file. > > * > > * Dump all fields for the given packet mbuf and all its associated > > > One more question, I don't see where this function is used in your > patchset. What is your use-case? > > Regards, > Olivier
This function is needed for crypto-perf application: http://dpdk.org/dev/patchwork/patch/17492/ to compare performance of crypto operations on segmented 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 = 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) == 0) linearize = 1; } // ... if (linearize) { /* PMD doesn't support scatter-gather and source buffer * is segmented. * We need to linearize it before enqueuing. */ for (i = 0; i < burst_size; i++) rte_pktmbuf_linearize(ops[i]->sym->m_src); } /* Enqueue burst of ops on crypto device */ ops_enqd = 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 straightforward. I will change it in v2. Thanks for suggestions Tomasz.
