[ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.
Darrell Ball
dlu998 at gmail.com
Wed Aug 8 16:06:19 UTC 2018
On Wed, Aug 8, 2018 at 3:17 AM, Ian Stokes <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>> 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 at intel.com <mailto:
>> tiago.lam at intel.com>>
>> > Acked-by: Eelco Chaudron <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