On Tue, Aug 7, 2018 at 5:08 AM, Stokes, Ian <[email protected]> wrote:
> > In its current implementation dp_packet_shift() is also unaware of multi- > > seg mbufs (that holds data in memory non-contiguously) and assumes that > > data exists contiguously in memory, memmove'ing data to perform the > shift. > > > > To add support for multi-seg mbuds a new set of functions was introduced, > > dp_packet_mbuf_shift() and dp_packet_mbuf_write(). These functions are > > used by dp_packet_shift(), when handling multi-seg mbufs, to shift and > > write data within a chain of mbufs. > > > > Hi Tiago, > > After applying this patch I see the following error during compilation. > > lib/conntrack.c: In function 'handle_ftp_ctl': > lib/conntrack.c:3154:11: error: 'addr_offset_from_ftp_data_start' may be > used uninitialized in this function [-Werror=maybe-uninitialized] > char *pkt_addr_str = ftp_data_start + addr_offset_from_ftp_data_ > start; > ^~~~~~~~~~~~ > lib/conntrack.c:3173:12: note: 'addr_offset_from_ftp_data_start' was > declared here > size_t addr_offset_from_ftp_data_start; > > It can be fixed with the following incremental. > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 974f985..7cd1fc9 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -3170,7 +3170,7 @@ handle_ftp_ctl(struct conntrack *ct, const struct > conn_lookup_ctx *ctx, > struct ip_header *l3_hdr = dp_packet_l3(pkt); > ovs_be32 v4_addr_rep = 0; > struct ct_addr v6_addr_rep; > - size_t addr_offset_from_ftp_data_start; > + size_t addr_offset_from_ftp_data_start = 0; > size_t addr_size = 0; > char *ftp_data_start; > bool do_seq_skew_adj = true; > > If there are no objections I can roll this into the patch upon application > to dpdk merge. > > Ian > hmm, I test with 4 major versions of GCC (4-7) with different flags, including O3 and I don't see these errors. I use 6.4 for the '6' major version What flags are you using ? I am building some versions from source; are you doing the same / Is it possible that your GCC 6.3.1 was not built correctly ? > > > Signed-off-by: Tiago Lam <[email protected]> > > Acked-by: Eelco Chaudron <[email protected]> > > --- > > lib/dp-packet.c | 97 > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > lib/dp-packet.h | 10 ++++++ > > 2 files changed, 107 insertions(+) > > > > diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 2aaeaae..d6e19eb > > 100644 > > --- a/lib/dp-packet.c > > +++ b/lib/dp-packet.c > > @@ -294,6 +294,97 @@ dp_packet_prealloc_headroom(struct dp_packet *b, > > size_t size) > > } > > } > > > > +#ifdef DPDK_NETDEV > > +/* Write len data bytes in a mbuf at specified offset. > > + * > > + * 'mbuf', pointer to the destination mbuf where 'ofs' is, and the mbuf > > +where > > + * the data will first be written. > > + * 'ofs', the offset within the provided 'mbuf' where 'data' is to be > > written. > > + * 'len', the size of the to be written 'data'. > > + * 'data', pointer to the to be written bytes. > > + * > > + * XXX: This function is the counterpart of the `rte_pktmbuf_read()` > > +function > > + * available with DPDK, in the rte_mbuf.h */ void > > +dp_packet_mbuf_write(struct rte_mbuf *mbuf, int16_t ofs, uint32_t len, > > + const void *data) > > +{ > > + char *dst_addr; > > + uint16_t data_len; > > + int len_copy; > > + while (mbuf) { > > + if (len == 0) { > > + break; > > + } > > + > > + dst_addr = rte_pktmbuf_mtod_offset(mbuf, char *, ofs); > > + data_len = MBUF_BUF_END(mbuf->buf_addr, mbuf->buf_len) - > > + dst_addr; > > + > > + len_copy = MIN(len, data_len); > > + /* We don't know if 'data' is the result of a rte_pktmbuf_read() > > call, > > + * in which case we may end up writing to the same region of > > memory we > > + * are reading from and overlapping. Hence the use of memmove() > > here */ > > + memmove(dst_addr, data, len_copy); > > + > > + data = ((char *) data) + len_copy; > > + len -= len_copy; > > + ofs = 0; > > + > > + mbuf = mbuf->next; > > + } > > +} > > + > > +static void > > +dp_packet_mbuf_shift_(struct rte_mbuf *dbuf, int16_t dst_ofs, > > + const struct rte_mbuf *sbuf, uint16_t src_ofs, > > +int len) { > > + char rd[len]; > > + const char *wd = rte_pktmbuf_read(sbuf, src_ofs, len, rd); > > + > > + ovs_assert(wd); > > + > > + dp_packet_mbuf_write(dbuf, dst_ofs, len, wd); } > > + > > +/* Similarly to dp_packet_shift(), shifts the data within the mbufs of > > +a > > + * dp_packet of DPBUF_DPDK source by 'delta' bytes. > > + * Caller must make sure of the following conditions: > > + * - When shifting left, delta can't be bigger than the data_len > > available in > > + * the last mbuf; > > + * - When shifting right, delta can't be bigger than the space available > > in the > > + * first mbuf (buf_len - data_off). > > + * Both these conditions guarantee that a shift operation doesn't fall > > +outside > > + * the bounds of the existing mbufs, so that the first and last mbufs > > +(when > > + * using multi-segment mbufs), remain the same. */ static void > > +dp_packet_mbuf_shift(struct dp_packet *b, int delta) { > > + uint16_t src_ofs; > > + int16_t dst_ofs; > > + > > + struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf); > > + struct rte_mbuf *tmbuf = rte_pktmbuf_lastseg(mbuf); > > + > > + if (delta < 0) { > > + ovs_assert(-delta <= tmbuf->data_len); > > + } else { > > + ovs_assert(delta < (mbuf->buf_len - mbuf->data_off)); > > + } > > + > > + /* Set the destination and source offsets to copy to */ > > + dst_ofs = delta; > > + src_ofs = 0; > > + > > + /* Shift data from src mbuf and offset to dst mbuf and offset */ > > + dp_packet_mbuf_shift_(mbuf, dst_ofs, mbuf, src_ofs, > > + rte_pktmbuf_pkt_len(mbuf)); > > + > > + /* Update mbufs' properties, and if using multi-segment mbufs, first > > and > > + * last mbuf's data_len also needs to be adjusted */ > > + mbuf->data_off = mbuf->data_off + dst_ofs; } #endif > > + > > /* Shifts all of the data within the allocated space in 'b' by 'delta' > > bytes. > > * For example, a 'delta' of 1 would cause each byte of data to move one > > byte > > * forward (from address 'p' to 'p+1'), and a 'delta' of -1 would cause > > each @@ -306,6 +397,12 @@ dp_packet_shift(struct dp_packet *b, int delta) > > : true); > > > > if (delta != 0) { > > +#ifdef DPDK_NETDEV > > + if (b->source == DPBUF_DPDK) { > > + dp_packet_mbuf_shift(b, delta); > > + return; > > + } > > +#endif > > char *dst = (char *) dp_packet_data(b) + delta; > > memmove(dst, dp_packet_data(b), dp_packet_size(b)); > > dp_packet_set_data(b, dst); > > diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 14e2551..6ca4e98 > > 100644 > > --- a/lib/dp-packet.h > > +++ b/lib/dp-packet.h > > @@ -80,6 +80,11 @@ struct dp_packet { > > }; > > }; > > > > +#ifdef DPDK_NETDEV > > +#define MBUF_BUF_END(BUF_ADDR, BUF_LEN) \ > > + (char *) (((char *) BUF_ADDR) + BUF_LEN) #endif > > + > > static inline void *dp_packet_data(const struct dp_packet *); static > > inline void dp_packet_set_data(struct dp_packet *, void *); static > inline > > void *dp_packet_base(const struct dp_packet *); @@ -133,6 +138,11 @@ > > static inline void *dp_packet_at(const struct dp_packet *, size_t offset, > > size_t size); static inline void > > *dp_packet_at_assert(const struct dp_packet *, > > size_t offset, size_t size); > > +#ifdef DPDK_NETDEV > > +void > > +dp_packet_mbuf_write(struct rte_mbuf *mbuf, int16_t ofs, uint32_t len, > > + const void *data); #endif > > static inline void *dp_packet_tail(const struct dp_packet *); static > > inline void *dp_packet_end(const struct dp_packet *); > > > > -- > > 2.7.4 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
