[ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.
Ian Stokes
ian.stokes at intel.com
Wed Aug 8 16:52:07 UTC 2018
On 8/8/2018 5:06 PM, Darrell Ball wrote:
>
>
> On Wed, Aug 8, 2018 at 3:17 AM, Ian Stokes <ian.stokes at intel.com
> <mailto:ian.stokes at 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 at intel.com <mailto:ian.stokes at intel.com>
> <mailto:ian.stokes at intel.com <mailto:ian.stokes at 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 at intel.com
> <mailto:tiago.lam at intel.com> <mailto:tiago.lam at intel.com
> <mailto:tiago.lam at intel.com>>>
> > Acked-by: Eelco Chaudron <echaudro at redhat.com
> <mailto:echaudro at redhat.com> <mailto:echaudro at redhat.com
> <mailto:echaudro at 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
>
>
>
>
More information about the dev
mailing list