On 03/10/2018 19:26, Flavio Leitner wrote: > On Fri, Sep 28, 2018 at 05:15:09PM +0100, Tiago Lam wrote: >> From: Michael Qiu <qiud...@chinac.com> >> >> When doing packet clone, if packet source is from DPDK driver, >> multi-segment must be considered, and copy the segment's data one by >> one. >> >> Also, lots of DPDK mbuf's info is missed during a copy, like packet >> type, ol_flags, etc. That information is very important for DPDK to do >> packets processing. >> >> Co-authored-by: Mark Kavanagh <mark.b.kavan...@intel.com> >> Co-authored-by: Tiago Lam <tiago....@intel.com> >> >> Signed-off-by: Michael Qiu <qiud...@chinac.com> >> Signed-off-by: Mark Kavanagh <mark.b.kavan...@intel.com> >> Signed-off-by: Tiago Lam <tiago....@intel.com> >> Acked-by: Eelco Chaudron <echau...@redhat.com> >> --- >> lib/dp-packet.c | 69 >> ++++++++++++++++++++++++++++++++++++++++++++++--------- >> lib/dp-packet.h | 3 +++ >> lib/netdev-dpdk.c | 1 + >> 3 files changed, 62 insertions(+), 11 deletions(-) >> >> diff --git a/lib/dp-packet.c b/lib/dp-packet.c >> index 167bf43..806640b 100644 >> --- a/lib/dp-packet.c >> +++ b/lib/dp-packet.c >> @@ -48,6 +48,22 @@ dp_packet_use__(struct dp_packet *b, void *base, size_t >> allocated, >> dp_packet_set_size(b, 0); >> } >> >> +#ifdef DPDK_NETDEV >> +void >> +dp_packet_copy_mbuf_flags(struct dp_packet *dst, const struct dp_packet >> *src) >> +{ >> + ovs_assert(dst != NULL && src != NULL); >> + struct rte_mbuf *buf_dst = &(dst->mbuf); >> + struct rte_mbuf buf_src = src->mbuf; >> + >> + buf_dst->ol_flags = buf_src.ol_flags; >> + buf_dst->packet_type = buf_src.packet_type; >> + buf_dst->tx_offload = buf_src.tx_offload; > > Nit: use dereferencing on both sides. > > Also, since this is a simple function that only copies few fields, maybe it > could > be in the dp-packet.h, so that netdev-dpdk.c can inline it. > >> +} >> +#else >> +#define dp_packet_copy_mbuf_flags(arg1, arg2) > > Nit2: since this is very specific to mbuf, we don't need to expose it > outside of DPDK. > >> +#endif >> + >> /* Initializes 'b' as an empty dp_packet that contains the 'allocated' >> bytes of >> * memory starting at 'base'. 'base' should be the first byte of a region >> * obtained from malloc(). It will be freed (with free()) if 'b' is >> resized or >> @@ -158,6 +174,44 @@ dp_packet_clone(const struct dp_packet *buffer) >> return dp_packet_clone_with_headroom(buffer, 0); >> } >> >> +#ifdef DPDK_NETDEV >> +struct dp_packet * >> +dp_packet_clone_with_headroom(const struct dp_packet *b, size_t headroom) { >> + struct dp_packet *new_buffer; >> + uint32_t pkt_len = dp_packet_size(b); >> + >> + /* copy multi-seg data */ >> + if (b->source == DPBUF_DPDK && !rte_pktmbuf_is_contiguous(&b->mbuf)) { >> + void *dst = NULL; >> + struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf); >> + >> + new_buffer = dp_packet_new_with_headroom(pkt_len, headroom); >> + dst = dp_packet_data(new_buffer); >> + dp_packet_set_size(new_buffer, pkt_len); >> + >> + if (!rte_pktmbuf_read(mbuf, 0, pkt_len, dst)) { > > Here we leak new_buffer which is xmalloc'ed.
Fixed. Thanks! > >> + return NULL; >> + } >> + } else { >> + new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(b), >> + dp_packet_size(b), >> + headroom); >> + } >> + >> + /* Copy the following fields into the returned buffer: l2_pad_size, >> + * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */ >> + memcpy(&new_buffer->l2_pad_size, &b->l2_pad_size, >> + sizeof(struct dp_packet) - >> + offsetof(struct dp_packet, l2_pad_size)); >> + >> + dp_packet_copy_mbuf_flags(new_buffer, b); >> + if (dp_packet_rss_valid(new_buffer)) { >> + new_buffer->mbuf.hash.rss = b->mbuf.hash.rss; >> + } >> + >> + return new_buffer; >> +} >> +#else >> /* Creates and returns a new dp_packet whose data are copied from 'buffer'. >> * The returned dp_packet will additionally have 'headroom' bytes of >> * headroom. */ >> @@ -165,32 +219,25 @@ struct dp_packet * >> dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t >> headroom) >> { >> struct dp_packet *new_buffer; >> + uint32_t pkt_len = dp_packet_size(buffer); >> >> new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer), >> - dp_packet_size(buffer), >> - headroom); >> + pkt_len, headroom); >> + >> /* Copy the following fields into the returned buffer: l2_pad_size, >> * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */ >> memcpy(&new_buffer->l2_pad_size, &buffer->l2_pad_size, >> sizeof(struct dp_packet) - >> offsetof(struct dp_packet, l2_pad_size)); >> >> -#ifdef DPDK_NETDEV >> - new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags; >> -#else >> new_buffer->rss_hash_valid = buffer->rss_hash_valid; >> -#endif >> - >> if (dp_packet_rss_valid(new_buffer)) { >> -#ifdef DPDK_NETDEV >> - new_buffer->mbuf.hash.rss = buffer->mbuf.hash.rss; >> -#else >> new_buffer->rss_hash = buffer->rss_hash; >> -#endif >> } >> >> return new_buffer; >> } >> +#endif > > Nit3: Looks like the above functions share some code which could be on > another function or maybe move the copy specifics to a separate > functions. The "nits" sound good to me. I'll include them in the next iteration. Tiago. > > >> /* Creates and returns a new dp_packet that initially contains a copy of the >> * 'size' bytes of data starting at 'data' with no headroom or tailroom. */ >> diff --git a/lib/dp-packet.h b/lib/dp-packet.h >> index aab8f62..cbf002c 100644 >> --- a/lib/dp-packet.h >> +++ b/lib/dp-packet.h >> @@ -124,6 +124,9 @@ void dp_packet_init_dpdk(struct dp_packet *); >> void dp_packet_init(struct dp_packet *, size_t); >> void dp_packet_uninit(struct dp_packet *); >> >> +void dp_packet_copy_mbuf_flags(struct dp_packet *dst, >> + const struct dp_packet *src); >> + >> struct dp_packet *dp_packet_new(size_t); >> struct dp_packet *dp_packet_new_with_headroom(size_t, size_t headroom); >> struct dp_packet *dp_packet_clone(const struct dp_packet *); >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 3b11546..8484239 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -2364,6 +2364,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct >> dp_packet_batch *batch) >> memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *), >> dp_packet_data(packet), size); >> dp_packet_set_size((struct dp_packet *)pkts[txcnt], size); >> + dp_packet_copy_mbuf_flags((struct dp_packet *)pkts[txcnt], packet); >> >> txcnt++; >> } >> -- >> 2.7.4 >> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev