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.
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 <[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
