Re: [ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Stokes, Ian]

> 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.
> 
> Signed-off-by: Tiago Lam 
> Acked-by: Eelco Chaudron 
> ---
>  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];

Above will break compilation with sparse.

lib/dp-packet.c:341:13: error: Variable length array is used.

Regards
Ian

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Ian Stokes]

On 8/8/2018 5:06 PM, Darrell Ball wrote:



On Wed, Aug 8, 2018 at 3:17 AM, Ian Stokes > wrote:


On 8/7/2018 6:13 PM, Darrell Ball wrote:



On Tue, Aug 7, 2018 at 5:08 AM, Stokes, Ian
mailto:ian.sto...@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 mailto:tiago@intel.com> >>
     > Acked-by: Eelco Chaudron mailto:echau...@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
   

Re: [ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Darrell Ball]

On Wed, Aug 8, 2018 at 3:17 AM, Ian Stokes  wrote:

> On 8/7/2018 6:13 PM, Darrell Ball wrote:
>
>
>>
>> On Tue, Aug 7, 2018 at 5:08 AM, Stokes, Ian > > 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@intel.com>>
>> > Acked-by: Eelco Chaudron > echau...@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;
>> 

Re: [ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Ian Stokes]

On 8/7/2018 6:13 PM, Darrell Ball wrote:



On Tue, Aug 7, 2018 at 5:08 AM, Stokes, Ian > 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 mailto:tiago@intel.com>>
> Acked-by: Eelco Chaudron mailto:echau...@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,

Re: [ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Darrell Ball]

On Tue, Aug 7, 2018 at 5:08 AM, Stokes, Ian  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 am building some versions from source; are you doing the same /

Is it possible that your GCC 6.3.1 was not built correctly ?




>
> > Signed-off-by: Tiago Lam 
> > Acked-by: Eelco Chaudron 
> > ---
> >  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 

Re: [ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Stokes, Ian]

> Hi Ian,
> 
> On 07/08/2018 13:08, Stokes, Ian 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
> >
> 
> I see no problem in including the incremental (I also see Darrell is
> already CC'ed, since this is in the userspace Conntrack).
> 

Agreed, would be interested in Darrells input here also.

> I noticed this myself during testing (are you perhaps using GCC 5.4?), but
> seems to be a false positive as when I looked there shouldn't be a path
> where `addr_offset_from_ftp_data_start` is used uninitialized, and when
> trying to compile with GCC 4.8 and GCC 7.3 there's no such warning.
> 

I tested it with gcc (GCC) 6.3.1. 

> But I agree the incremental is a better practice.
> 
> Thanks,
> Tiago.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Lam, Tiago]

Hi Ian,

On 07/08/2018 13:08, Stokes, Ian 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
> 

I see no problem in including the incremental (I also see Darrell is
already CC'ed, since this is in the userspace Conntrack).

I noticed this myself during testing (are you perhaps using GCC 5.4?),
but seems to be a false positive as when I looked there shouldn't be a
path where `addr_offset_from_ftp_data_start` is used uninitialized, and
when trying to compile with GCC 4.8 and GCC 7.3 there's no such warning.

But I agree the incremental is a better practice.

Thanks,
Tiago.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Stokes, Ian]

> 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

> Signed-off-by: Tiago Lam 
> Acked-by: Eelco Chaudron 
> ---
>  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 *, >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 

[ovs-dev] [PATCH v7 07/13] dp-packet: Handle multi-seg mubfs in shift() func.

[Tiago Lam]

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.

Signed-off-by: Tiago Lam 
Acked-by: Eelco Chaudron 
---
 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 *, >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)