| Message ID | 1532528359-96999-8-git-send-email-tiago.lam@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ian Stokes |
| Headers | show |
| Series | Support multi-segment mbufs | expand |
> 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 > Signed-off-by: Tiago Lam <tiago.lam@intel.com> > Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > 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
Hi Ian, On 07/08/2018 13:08, Stokes, Ian 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 > I see no problem in including the incremental (I also see Darrell is already CC'ed, since this is in the userspace Conntrack). I noticed this myself during testing (are you perhaps using GCC 5.4?), but seems to be a false positive as when I looked there shouldn't be a path where `addr_offset_from_ftp_data_start` is used uninitialized, and when trying to compile with GCC 4.8 and GCC 7.3 there's no such warning. But I agree the incremental is a better practice. Thanks, Tiago.
> Hi Ian, > > On 07/08/2018 13:08, Stokes, Ian 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 > > > > I see no problem in including the incremental (I also see Darrell is > already CC'ed, since this is in the userspace Conntrack). > Agreed, would be interested in Darrells input here also. > I noticed this myself during testing (are you perhaps using GCC 5.4?), but > seems to be a false positive as when I looked there shouldn't be a path > where `addr_offset_from_ftp_data_start` is used uninitialized, and when > trying to compile with GCC 4.8 and GCC 7.3 there's no such warning. > I tested it with gcc (GCC) 6.3.1. > But I agree the incremental is a better practice. > > Thanks, > Tiago.
On Tue, Aug 7, 2018 at 5:08 AM, Stokes, Ian <ian.stokes@intel.com> 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 <tiago.lam@intel.com> > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > --- > > 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 > >
On 8/7/2018 6:13 PM, Darrell Ball wrote: > > > On Tue, Aug 7, 2018 at 5:08 AM, Stokes, Ian <ian.stokes@intel.com > <mailto:ian.stokes@intel.com>> 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've compiled with and without the following flags -g -Ofast -march=native. In both cases the warning still occurs. I'm using gcc (GCC) 6.3.1. Tiago also spotted the same issue occurring on GCC 5.4 so it doesn't seem isolated to 6.3.1. > > 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 ? I'm not building gcc from source, I'm using Fedora 24 and the GCC installed from the fedora repo. Ian > > > > > Signed-off-by: Tiago Lam <tiago.lam@intel.com <mailto:tiago.lam@intel.com>> > > Acked-by: Eelco Chaudron <echaudro@redhat.com <mailto:echaudro@redhat.com>> > > --- > > 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 > >
On Wed, Aug 8, 2018 at 3:17 AM, Ian Stokes <ian.stokes@intel.com> wrote: > On 8/7/2018 6:13 PM, Darrell Ball wrote: > > >> >> On Tue, Aug 7, 2018 at 5:08 AM, Stokes, Ian <ian.stokes@intel.com >> <mailto:ian.stokes@intel.com>> 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've compiled with and without the following flags -g -Ofast -march=native. > I use -g and -march=native. But not -Ofast, since it uses non-standard optimizations and did not find it benefits much. I doubt any base gcc version would not be able to understand branching, If anything, a non-standard optimization level would be the only impactful difference. > > In both cases the warning still occurs. > > I'm using gcc (GCC) 6.3.1. Tiago also spotted the same issue occurring on > GCC 5.4 so it doesn't seem isolated to 6.3.1. I have a VM with 5.4.0 and also added 6.3.0 on another VM, but don't an issue with "-g -Ofast -march=native" I did not find the sources for 6.3.1, at least easily and I doubt I will be installing 2 year old Fedora to get 6.3.1 anytime soon :-) If this is causing you grieve and you don't want to use a more recent gcc, initialization is ok for 'infrequent code paths' such as we discuss here. > > > > >> 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 ? >> > > I'm not building gcc from source, I'm using Fedora 24 and the GCC > installed from the fedora repo. > > Ian > >> >> >> >> > Signed-off-by: Tiago Lam <tiago.lam@intel.com <mailto: >> tiago.lam@intel.com>> >> > Acked-by: Eelco Chaudron <echaudro@redhat.com <mailto: >> echaudro@redhat.com>> >> >> > --- >> > 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 >> >> >> >
On 8/8/2018 5:06 PM, Darrell Ball wrote: > > > On Wed, Aug 8, 2018 at 3:17 AM, Ian Stokes <ian.stokes@intel.com > <mailto:ian.stokes@intel.com>> wrote: > > On 8/7/2018 6:13 PM, Darrell Ball wrote: > > > > On Tue, Aug 7, 2018 at 5:08 AM, Stokes, Ian > <ian.stokes@intel.com <mailto:ian.stokes@intel.com> > <mailto:ian.stokes@intel.com <mailto:ian.stokes@intel.com>>> 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've compiled with and without the following flags -g -Ofast > -march=native. > > > I use -g and -march=native. > But not -Ofast, since it uses non-standard optimizations and did not > find it benefits much. Good point, I've seen some issues with it in the past, it's more of an artifact from the OVS docs at this stage. > > I doubt any base gcc version would not be able to understand branching, > If anything, a non-standard optimization level would be the only > impactful difference. > > > In both cases the warning still occurs. > > I'm using gcc (GCC) 6.3.1. Tiago also spotted the same issue > occurring on GCC 5.4 so it doesn't seem isolated to 6.3.1. > > > > I have a VM with 5.4.0 and also added 6.3.0 on another VM, but don't an > issue with "-g -Ofast -march=native" > I did not find the sources for 6.3.1, at least easily and I doubt I will > be installing 2 year old Fedora to get 6.3.1 anytime soon :-) :), Yes an upgrade to a newer target is called for at this point, was hoping to get around to it after the 2.10 release. > > If this is causing you grieve and you don't want to use a more recent > gcc, initialization is ok for 'infrequent code paths' such as we > discuss here. Thanks for the input Darrell, I'll roll in the change so to the patch to keep compilation for gcc similarly affected. Thanks Ian > > > > > > 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 ? > > > I'm not building gcc from source, I'm using Fedora 24 and the GCC > installed from the fedora repo. > > Ian > > > > > > Signed-off-by: Tiago Lam <tiago.lam@intel.com > <mailto:tiago.lam@intel.com> <mailto:tiago.lam@intel.com > <mailto:tiago.lam@intel.com>>> > > Acked-by: Eelco Chaudron <echaudro@redhat.com > <mailto:echaudro@redhat.com> <mailto:echaudro@redhat.com > <mailto:echaudro@redhat.com>>> > > > --- > > 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 > > > >
> 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. > > Signed-off-by: Tiago Lam <tiago.lam@intel.com> > Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > 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]; Above will break compilation with sparse. lib/dp-packet.c:341:13: error: Variable length array is used. Regards Ian
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 *);