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