On Wed, Aug 8, 2018 at 3:17 AM, Ian Stokes <[email protected]> wrote:
> On 8/7/2018 6:13 PM, Darrell Ball wrote: > > >> >> On Tue, Aug 7, 2018 at 5:08 AM, Stokes, Ian <[email protected] >> <mailto:[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'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 <[email protected] <mailto: >> [email protected]>> >> > Acked-by: Eelco Chaudron <[email protected] <mailto: >> [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
