Message ID | 1528734090-220990-12-git-send-email-tiago.lam@intel.com |
---|---|
State | Superseded |
Headers | show |
Series | Support multi-segment mbufs | expand |
On 11 Jun 2018, at 18:21, Tiago Lam wrote: > From: Michael Qiu <qiudayu@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.kavanagh@intel.com> > > Signed-off-by: Michael Qiu <qiudayu@chinac.com> > Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> > --- > lib/dp-packet.c | 76 > +++++++++++++++++++++++++++++++++++++++++++++++-------- > lib/dp-packet.h | 3 +++ > lib/netdev-dpdk.c | 1 + > 3 files changed, 69 insertions(+), 11 deletions(-) > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c > index d0fab94..2e65b82 100644 > --- a/lib/dp-packet.c > +++ b/lib/dp-packet.c > @@ -48,6 +48,23 @@ 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->nb_segs = buf_src.nb_segs; > + buf_dst->ol_flags = buf_src.ol_flags; > + buf_dst->packet_type = buf_src.packet_type; > + buf_dst->tx_offload = buf_src.tx_offload; > +} > +#else > +#define dp_packet_copy_mbuf_flags(arg1, arg2) > +#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 +175,50 @@ 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 *buffer, > + size_t headroom) { > + struct dp_packet *new_buffer; > + uint32_t pkt_len = dp_packet_size(buffer); > + > + /* copy multi-seg data */ > + if (buffer->source == DPBUF_DPDK && buffer->mbuf.nb_segs > 1) { > + uint32_t offset = 0; > + void *dst = NULL; > + struct rte_mbuf *tmbuf = CONST_CAST(struct rte_mbuf *, > + &(buffer->mbuf)); > + > + new_buffer = dp_packet_new_with_headroom(pkt_len, headroom); > + dp_packet_set_size(new_buffer, pkt_len + headroom); > + dst = dp_packet_tail(new_buffer); > + The dst is not pointing to the first data, but to the end of the data, as dp_packet_set_size() has been called. Why not just call dst = dp_packet_put_uninit(). > + while (tmbuf) { > + rte_memcpy((char *)dst + offset, > + rte_pktmbuf_mtod(tmbuf, void *), > tmbuf->data_len); > + offset += tmbuf->data_len; > + tmbuf = tmbuf->next; Why not using rte_pktmbuf_read() here with a check its not a contiguous read? > + } > + } else { > + new_buffer = > dp_packet_clone_data_with_headroom(dp_packet_data(buffer), > + > dp_packet_size(buffer), > + 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)); > + > + dp_packet_copy_mbuf_flags(new_buffer, buffer); > + if (dp_packet_rss_valid(new_buffer)) { > + new_buffer->mbuf.hash.rss = buffer->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 +226,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 > > /* 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 4946fa3..3852756 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 efd7c20..9b1fb9a 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2215,6 +2215,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
On 18/06/2018 14:10, Eelco Chaudron wrote: > > > On 11 Jun 2018, at 18:21, Tiago Lam wrote: > >> From: Michael Qiu <qiudayu@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.kavanagh@intel.com> >> >> Signed-off-by: Michael Qiu <qiudayu@chinac.com> >> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> >> --- >> lib/dp-packet.c | 76 >> +++++++++++++++++++++++++++++++++++++++++++++++-------- >> lib/dp-packet.h | 3 +++ >> lib/netdev-dpdk.c | 1 + >> 3 files changed, 69 insertions(+), 11 deletions(-) >> >> diff --git a/lib/dp-packet.c b/lib/dp-packet.c >> index d0fab94..2e65b82 100644 >> --- a/lib/dp-packet.c >> +++ b/lib/dp-packet.c >> @@ -48,6 +48,23 @@ 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->nb_segs = buf_src.nb_segs; >> + buf_dst->ol_flags = buf_src.ol_flags; >> + buf_dst->packet_type = buf_src.packet_type; >> + buf_dst->tx_offload = buf_src.tx_offload; >> +} >> +#else >> +#define dp_packet_copy_mbuf_flags(arg1, arg2) >> +#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 +175,50 @@ 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 *buffer, >> + size_t headroom) { >> + struct dp_packet *new_buffer; >> + uint32_t pkt_len = dp_packet_size(buffer); >> + >> + /* copy multi-seg data */ >> + if (buffer->source == DPBUF_DPDK && buffer->mbuf.nb_segs > 1) { >> + uint32_t offset = 0; >> + void *dst = NULL; >> + struct rte_mbuf *tmbuf = CONST_CAST(struct rte_mbuf *, >> + &(buffer->mbuf)); >> + >> + new_buffer = dp_packet_new_with_headroom(pkt_len, headroom); >> + dp_packet_set_size(new_buffer, pkt_len + headroom); >> + dst = dp_packet_tail(new_buffer); >> + > > The dst is not pointing to the first data, but to the end of the data, > as dp_packet_set_size() has been called. Why not just call dst = > dp_packet_put_uninit(). I'm taking your suggestion below, so this will be removed. > >> + while (tmbuf) { >> + rte_memcpy((char *)dst + offset, >> + rte_pktmbuf_mtod(tmbuf, void *), >> tmbuf->data_len); >> + offset += tmbuf->data_len; >> + tmbuf = tmbuf->next; > > Why not using rte_pktmbuf_read() here with a check its not a contiguous > read? > Good point! We can use `rte_pktmbuf_read()` to read the data to `new_buffer`. It should be OK since in this clause we have always more than 1 mbuf in the chain. Tiago.
On 22 Jun 2018, at 21:05, Lam, Tiago wrote: > On 18/06/2018 14:10, Eelco Chaudron wrote: >> >> >> On 11 Jun 2018, at 18:21, Tiago Lam wrote: >> >>> From: Michael Qiu <qiudayu@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.kavanagh@intel.com> >>> >>> Signed-off-by: Michael Qiu <qiudayu@chinac.com> >>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com> >>> --- >>> lib/dp-packet.c | 76 >>> +++++++++++++++++++++++++++++++++++++++++++++++-------- >>> lib/dp-packet.h | 3 +++ >>> lib/netdev-dpdk.c | 1 + >>> 3 files changed, 69 insertions(+), 11 deletions(-) >>> >>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c >>> index d0fab94..2e65b82 100644 >>> --- a/lib/dp-packet.c >>> +++ b/lib/dp-packet.c >>> @@ -48,6 +48,23 @@ 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->nb_segs = buf_src.nb_segs; >>> + buf_dst->ol_flags = buf_src.ol_flags; >>> + buf_dst->packet_type = buf_src.packet_type; >>> + buf_dst->tx_offload = buf_src.tx_offload; >>> +} >>> +#else >>> +#define dp_packet_copy_mbuf_flags(arg1, arg2) >>> +#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 +175,50 @@ 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 *buffer, >>> + size_t headroom) { >>> + struct dp_packet *new_buffer; >>> + uint32_t pkt_len = dp_packet_size(buffer); >>> + >>> + /* copy multi-seg data */ >>> + if (buffer->source == DPBUF_DPDK && buffer->mbuf.nb_segs > 1) { >>> + uint32_t offset = 0; >>> + void *dst = NULL; >>> + struct rte_mbuf *tmbuf = CONST_CAST(struct rte_mbuf *, >>> + &(buffer->mbuf)); >>> + >>> + new_buffer = dp_packet_new_with_headroom(pkt_len, headroom); >>> + dp_packet_set_size(new_buffer, pkt_len + headroom); >>> + dst = dp_packet_tail(new_buffer); >>> + >> >> The dst is not pointing to the first data, but to the end of the data, >> as dp_packet_set_size() has been called. Why not just call dst = >> dp_packet_put_uninit(). > > I'm taking your suggestion below, so this will be removed. > >> >>> + while (tmbuf) { >>> + rte_memcpy((char *)dst + offset, >>> + rte_pktmbuf_mtod(tmbuf, void *), >>> tmbuf->data_len); >>> + offset += tmbuf->data_len; >>> + tmbuf = tmbuf->next; >> >> Why not using rte_pktmbuf_read() here with a check its not a contiguous >> read? >> > > Good point! We can use `rte_pktmbuf_read()` to read the data to > `new_buffer`. It should be OK since in this clause we have always more > than 1 mbuf in the chain. Please add a check to be sure!
diff --git a/lib/dp-packet.c b/lib/dp-packet.c index d0fab94..2e65b82 100644 --- a/lib/dp-packet.c +++ b/lib/dp-packet.c @@ -48,6 +48,23 @@ 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->nb_segs = buf_src.nb_segs; + buf_dst->ol_flags = buf_src.ol_flags; + buf_dst->packet_type = buf_src.packet_type; + buf_dst->tx_offload = buf_src.tx_offload; +} +#else +#define dp_packet_copy_mbuf_flags(arg1, arg2) +#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 +175,50 @@ 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 *buffer, + size_t headroom) { + struct dp_packet *new_buffer; + uint32_t pkt_len = dp_packet_size(buffer); + + /* copy multi-seg data */ + if (buffer->source == DPBUF_DPDK && buffer->mbuf.nb_segs > 1) { + uint32_t offset = 0; + void *dst = NULL; + struct rte_mbuf *tmbuf = CONST_CAST(struct rte_mbuf *, + &(buffer->mbuf)); + + new_buffer = dp_packet_new_with_headroom(pkt_len, headroom); + dp_packet_set_size(new_buffer, pkt_len + headroom); + dst = dp_packet_tail(new_buffer); + + while (tmbuf) { + rte_memcpy((char *)dst + offset, + rte_pktmbuf_mtod(tmbuf, void *), tmbuf->data_len); + offset += tmbuf->data_len; + tmbuf = tmbuf->next; + } + } else { + new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer), + dp_packet_size(buffer), + 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)); + + dp_packet_copy_mbuf_flags(new_buffer, buffer); + if (dp_packet_rss_valid(new_buffer)) { + new_buffer->mbuf.hash.rss = buffer->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 +226,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 /* 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 4946fa3..3852756 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 efd7c20..9b1fb9a 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2215,6 +2215,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++; }