[dpdk-dev] [PATCH 0/2] 21.05 fixes for OVS

[dpdk-dev] [PATCH 0/2] 21.05 fixes for OVS

[David Marchand]

Testing OVS build against 21.05-rc2 revealed some issues.
Bits were also missing on OVS side, so for people interested, a
rebased dpdk-latest OVS branch is available at:
https://github.com/david-marchand/ovs/commits/dpdk-latest


-- 
David Marchand

David Marchand (2):
  net: add endianness annotations to ethernet headers
  net: fix header include order for FreeBSD

 lib/net/rte_ether.h | 6 +++---
 lib/net/rte_ip.h    | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.23.0

[dpdk-dev] [PATCH 1/2] net: add endianness annotations to ethernet headers

[David Marchand]

Spotted by sparse in OVS build:

/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:789:27:
error: incorrect type in initializer (different base types)
/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:789:27:
expected unsigned short [usertype] ether_type
/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:789:27:
got restricted ovs_be16 [usertype]
/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:829:25:
error: incorrect type in initializer (different base types)
/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:829:25:
expected unsigned short [usertype] vlan_tci
/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:829:25:
got restricted ovs_be16 [usertype]
/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:830:26:
error: incorrect type in initializer (different base types)
/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:830:26:
expected unsigned short [usertype] eth_proto
/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:830:26:
got restricted ovs_be16 [usertype]

This was not caught before as no code in headers was using those fields.
This changed with commit 6f2168b69aee ("ethdev: reuse ethernet header
definition in flow item") and commit a56a262e3408 ("ethdev: reuse VLAN
header definition in flow item").

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/net/rte_ether.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/net/rte_ether.h b/lib/net/rte_ether.h
index a303c24a8c..7ee5e9a292 100644
--- a/lib/net/rte_ether.h
+++ b/lib/net/rte_ether.h
@@ -276,7 +276,7 @@ struct rte_ether_hdr {
 			struct rte_ether_addr S_addr;
 		} S_un; /**< Do not use directly; use s_addr instead.*/
 	};
-	uint16_t ether_type; /**< Frame type. */
+	rte_be16_t ether_type; /**< Frame type. */
 } __rte_aligned(2);
 
 #pragma pop_macro("s_addr")
@@ -287,8 +287,8 @@ struct rte_ether_hdr {
  * of the encapsulated frame.
  */
 struct rte_vlan_hdr {
-	uint16_t vlan_tci; /**< Priority (3) + CFI (1) + Identifier Code (12) */
-	uint16_t eth_proto;/**< Ethernet type of encapsulated frame. */
+	rte_be16_t vlan_tci;  /**< Priority (3) + CFI (1) + Identifier Code (12) */
+	rte_be16_t eth_proto; /**< Ethernet type of encapsulated frame. */
 } __rte_packed;
 
 
-- 
2.23.0

Re: [dpdk-dev] [PATCH 1/2] net: add endianness annotations to ethernet headers

[Olivier Matz]

On Thu, May 06, 2021 at 05:14:25PM +0200, David Marchand wrote:
> Spotted by sparse in OVS build:
> 
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:789:27:
> error: incorrect type in initializer (different base types)
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:789:27:
> expected unsigned short [usertype] ether_type
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:789:27:
> got restricted ovs_be16 [usertype]
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:829:25:
> error: incorrect type in initializer (different base types)
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:829:25:
> expected unsigned short [usertype] vlan_tci
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:829:25:
> got restricted ovs_be16 [usertype]
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:830:26:
> error: incorrect type in initializer (different base types)
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:830:26:
> expected unsigned short [usertype] eth_proto
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h:830:26:
> got restricted ovs_be16 [usertype]
> 
> This was not caught before as no code in headers was using those fields.
> This changed with commit 6f2168b69aee ("ethdev: reuse ethernet header
> definition in flow item") and commit a56a262e3408 ("ethdev: reuse VLAN
> header definition in flow item").
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

I was wondering if it wouldn't be safer to change the initializer
instead of the structure definition, to avoid any API breakage.

But after some off-list discussions, it appears that there is no API
breakage (rte_be16_t is a typedef to uint16_t), and this kind of changes
was already done in the past.

Acked-by: Olivier Matz <olivier.matz@6wind.com>

[dpdk-dev] [PATCH 2/2] net: fix header include order for FreeBSD

[David Marchand]

Spotted by sparse in OVS build:
../../lib/netdev-dpdk.c: note: in included file (through
/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_ip.h,
/home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h, ...):
../../include/sparse/arpa/inet.h:22:2: error: "Must include
<netinet/in.h> before <arpa/inet.h> for FreeBSD support"

This is a check enforced by OVS itself.
See [1] for some context.

1: https://github.com/openvswitch/ovs/commit/b2befd5bb2db

Fixes: 89813a522e68 ("net: provide IP-related API on any OS")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/net/rte_ip.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 8c189009b0..4b728969c1 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -22,8 +22,8 @@
 #else
 #include <sys/socket.h>
 #include <sys/types.h>
-#include <arpa/inet.h>
 #include <netinet/in.h>
+#include <arpa/inet.h>
 #include <netinet/ip.h>
 #endif
 
-- 
2.23.0

Re: [dpdk-dev] [PATCH 2/2] net: fix header include order for FreeBSD

[Dmitry Kozlyuk]

2021-05-06 17:14 (UTC+0200), David Marchand:
> Spotted by sparse in OVS build:
> ../../lib/netdev-dpdk.c: note: in included file (through
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_ip.h,
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h, ...):
> ../../include/sparse/arpa/inet.h:22:2: error: "Must include
> <netinet/in.h> before <arpa/inet.h> for FreeBSD support"
> 
> This is a check enforced by OVS itself.
> See [1] for some context.
> 
> 1: https://github.com/openvswitch/ovs/commit/b2befd5bb2db
> 
> Fixes: 89813a522e68 ("net: provide IP-related API on any OS")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/net/rte_ip.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> index 8c189009b0..4b728969c1 100644
> --- a/lib/net/rte_ip.h
> +++ b/lib/net/rte_ip.h
> @@ -22,8 +22,8 @@
>  #else
>  #include <sys/socket.h>
>  #include <sys/types.h>
> -#include <arpa/inet.h>
>  #include <netinet/in.h>
> +#include <arpa/inet.h>
>  #include <netinet/ip.h>
>  #endif
>

Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

It would be interesting to know exact issue this solves for OVS.
Referenced commit only says FreeBSD "insists" on this include order,
but DPDK and standalone files with these includes build either way.

Re: [dpdk-dev] [PATCH 2/2] net: fix header include order for FreeBSD

[David Marchand]

On Fri, May 7, 2021 at 12:24 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> 2021-05-06 17:14 (UTC+0200), David Marchand:
> > Spotted by sparse in OVS build:
> > ../../lib/netdev-dpdk.c: note: in included file (through
> > /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_ip.h,
> > /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h, ...):
> > ../../include/sparse/arpa/inet.h:22:2: error: "Must include
> > <netinet/in.h> before <arpa/inet.h> for FreeBSD support"
> >
> > This is a check enforced by OVS itself.
> > See [1] for some context.
> >
> > 1: https://github.com/openvswitch/ovs/commit/b2befd5bb2db
> >
> > Fixes: 89813a522e68 ("net: provide IP-related API on any OS")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> >  lib/net/rte_ip.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > index 8c189009b0..4b728969c1 100644
> > --- a/lib/net/rte_ip.h
> > +++ b/lib/net/rte_ip.h
> > @@ -22,8 +22,8 @@
> >  #else
> >  #include <sys/socket.h>
> >  #include <sys/types.h>
> > -#include <arpa/inet.h>
> >  #include <netinet/in.h>
> > +#include <arpa/inet.h>
> >  #include <netinet/ip.h>
> >  #endif
> >
>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
>
> It would be interesting to know exact issue this solves for OVS.
> Referenced commit only says FreeBSD "insists" on this include order,
> but DPDK and standalone files with these includes build either way.

Indeed, I tried building with FreeBSD 13.0 and I can see no pb.
This might be something that has been fixed in FreeBSD.

Ben, would you have details on this header inclusion order?


-- 
David Marchand

Re: [dpdk-dev] [PATCH 2/2] net: fix header include order for FreeBSD

[Ben Pfaff]

On Fri, May 07, 2021 at 11:06:38AM +0200, David Marchand wrote:
> On Fri, May 7, 2021 at 12:24 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> >
> > 2021-05-06 17:14 (UTC+0200), David Marchand:
> > > Spotted by sparse in OVS build:
> > > ../../lib/netdev-dpdk.c: note: in included file (through
> > > /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_ip.h,
> > > /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h, ...):
> > > ../../include/sparse/arpa/inet.h:22:2: error: "Must include
> > > <netinet/in.h> before <arpa/inet.h> for FreeBSD support"
> > >
> > > This is a check enforced by OVS itself.
> > > See [1] for some context.
> > >
> > > 1: https://github.com/openvswitch/ovs/commit/b2befd5bb2db
> > >
> > > Fixes: 89813a522e68 ("net: provide IP-related API on any OS")
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > >  lib/net/rte_ip.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > > index 8c189009b0..4b728969c1 100644
> > > --- a/lib/net/rte_ip.h
> > > +++ b/lib/net/rte_ip.h
> > > @@ -22,8 +22,8 @@
> > >  #else
> > >  #include <sys/socket.h>
> > >  #include <sys/types.h>
> > > -#include <arpa/inet.h>
> > >  #include <netinet/in.h>
> > > +#include <arpa/inet.h>
> > >  #include <netinet/ip.h>
> > >  #endif
> > >
> >
> > Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> >
> > It would be interesting to know exact issue this solves for OVS.
> > Referenced commit only says FreeBSD "insists" on this include order,
> > but DPDK and standalone files with these includes build either way.
> 
> Indeed, I tried building with FreeBSD 13.0 and I can see no pb.
> This might be something that has been fixed in FreeBSD.
> 
> Ben, would you have details on this header inclusion order?

Using the wrong order caused a compiler error on whatever version of
FreeBSD was current at the time of those commits.  I think it was a
historical BSD issue, since I remember running into this for many years
across multiple BSD versions.

If it's fixed now, we can drop the constraint.

Re: [dpdk-dev] [PATCH 2/2] net: fix header include order for FreeBSD

[Ben Pfaff]

On Fri, May 07, 2021 at 12:12:09PM -0700, Ben Pfaff wrote:
> On Fri, May 07, 2021 at 11:06:38AM +0200, David Marchand wrote:
> > On Fri, May 7, 2021 at 12:24 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
> > >
> > > 2021-05-06 17:14 (UTC+0200), David Marchand:
> > > > Spotted by sparse in OVS build:
> > > > ../../lib/netdev-dpdk.c: note: in included file (through
> > > > /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_ip.h,
> > > > /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h, ...):
> > > > ../../include/sparse/arpa/inet.h:22:2: error: "Must include
> > > > <netinet/in.h> before <arpa/inet.h> for FreeBSD support"
> > > >
> > > > This is a check enforced by OVS itself.
> > > > See [1] for some context.
> > > >
> > > > 1: https://github.com/openvswitch/ovs/commit/b2befd5bb2db
> > > >
> > > > Fixes: 89813a522e68 ("net: provide IP-related API on any OS")
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > ---
> > > >  lib/net/rte_ip.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
> > > > index 8c189009b0..4b728969c1 100644
> > > > --- a/lib/net/rte_ip.h
> > > > +++ b/lib/net/rte_ip.h
> > > > @@ -22,8 +22,8 @@
> > > >  #else
> > > >  #include <sys/socket.h>
> > > >  #include <sys/types.h>
> > > > -#include <arpa/inet.h>
> > > >  #include <netinet/in.h>
> > > > +#include <arpa/inet.h>
> > > >  #include <netinet/ip.h>
> > > >  #endif
> > > >
> > >
> > > Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > >
> > > It would be interesting to know exact issue this solves for OVS.
> > > Referenced commit only says FreeBSD "insists" on this include order,
> > > but DPDK and standalone files with these includes build either way.
> > 
> > Indeed, I tried building with FreeBSD 13.0 and I can see no pb.
> > This might be something that has been fixed in FreeBSD.
> > 
> > Ben, would you have details on this header inclusion order?
> 
> Using the wrong order caused a compiler error on whatever version of
> FreeBSD was current at the time of those commits.  I think it was a
> historical BSD issue, since I remember running into this for many years
> across multiple BSD versions.
> 
> If it's fixed now, we can drop the constraint.

Oh, Mac OS X appears to also have this or a related problem:

commit ff6aa424ef1f46d2d1c468940219e187632ec894
Author: Borja Marcos EA2EKH <borjam@gmail.com>
Date:   Mon Nov 6 10:32:12 2017 +0100

    conntrack: Include <sys/types.h> before <netinet/icmp6.h>.
    
    FreeBSD and Mac OS X require this.
    
    Signed-off-by: Ben Pfaff <blp@ovn.org>

Re: [dpdk-dev] [PATCH 2/2] net: fix header include order for FreeBSD

[David Marchand]

Hello Ben,

On Fri, May 7, 2021 at 9:13 PM Ben Pfaff <blp@ovn.org> wrote:
> > > > It would be interesting to know exact issue this solves for OVS.
> > > > Referenced commit only says FreeBSD "insists" on this include order,
> > > > but DPDK and standalone files with these includes build either way.
> > >
> > > Indeed, I tried building with FreeBSD 13.0 and I can see no pb.
> > > This might be something that has been fixed in FreeBSD.
> > >
> > > Ben, would you have details on this header inclusion order?
> >
> > Using the wrong order caused a compiler error on whatever version of
> > FreeBSD was current at the time of those commits.  I think it was a
> > historical BSD issue, since I remember running into this for many years
> > across multiple BSD versions.
> >
> > If it's fixed now, we can drop the constraint.

I reverted some bits on arpa/inet.h vs netinet/in.h headers in OVS but
did not reproduce the issue on FreeBSD 11 and 12 with Cirrus.


>
> Oh, Mac OS X appears to also have this or a related problem:

Reverting the patch on conntrack which deals with sys/types.h vs
netinet/icmp6.h, there is an issue on FreeBSD 11 and 12:
https://cirrus-ci.com/task/4786810854309888

In file included from lib/conntrack.c:libtool: compile: clang
-DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib
-I/usr/local/include -Wstrict-prototypes -Wall -Wextra
-Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security
-Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align
-Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes
-Wmissing-field-initializers -Wthread-safety -fno-strict-aliasing
-Wswitch-bool -Wlogical-not-parentheses -Wsizeof-array-argument
-Wshift-negative-value -Qunused-arguments -Wshadow
-Wno-null-pointer-arithmetic -Warray-bounds-pointer-arithmetic -Werror
-Werror -g -O2 -Wall -DHAVE_AVX512F -DHAVE_LD_AVX512_GOOD -MT
lib/coverage.lo -MD -MP -MF lib/.deps/coverage.Tpo -c lib/coverage.c
-o lib/coverage.o
21:
/usr/include/netinet/icmp6.h:71:2: error: unknown type name 'u_int8_t'
u_int8_t icmp6_type; /* type field */
^
/usr/include/netinet/icmp6.h:72:2: error: unknown type name 'u_int8_t'
u_int8_t icmp6_code; /* code field */
^
...


I don't think it is worth dropping only the first constraint, so I
would keep it as is in OVS.
Thanks.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 2/2] net: fix header include order for FreeBSD

[Olivier Matz]

On Thu, May 06, 2021 at 05:14:26PM +0200, David Marchand wrote:
> Spotted by sparse in OVS build:
> ../../lib/netdev-dpdk.c: note: in included file (through
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_ip.h,
> /home/runner/work/ovs/ovs/dpdk-dir/build/include/rte_flow.h, ...):
> ../../include/sparse/arpa/inet.h:22:2: error: "Must include
> <netinet/in.h> before <arpa/inet.h> for FreeBSD support"
> 
> This is a check enforced by OVS itself.
> See [1] for some context.
> 
> 1: https://github.com/openvswitch/ovs/commit/b2befd5bb2db
> 
> Fixes: 89813a522e68 ("net: provide IP-related API on any OS")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH 0/2] 21.05 fixes for OVS

[David Marchand]

On Thu, May 6, 2021 at 5:15 PM David Marchand <david.marchand@redhat.com> wrote:
>
> Testing OVS build against 21.05-rc2 revealed some issues.
> Bits were also missing on OVS side, so for people interested, a
> rebased dpdk-latest OVS branch is available at:
> https://github.com/david-marchand/ovs/commits/dpdk-latest

I got failures for a "dynamic_config" job running on the Mellanox
server from the lab.
I don't see how my patches could break this.

Max segment number per MTU/TSO: 40
Switch name: 0000:05:00.0
Switch domain Id: 0
Switch Port Id: 65535
06/05/2021 14:03:20              TestDynamicConfig: set_up_all failed:
Traceback (most recent call last):
  File "/home/jenkins/dts-func/framework/test_case.py", line 258, in
execute_setup_all
    self.set_up_all()
  File "tests/TestSuite_dynamic_config.py", line 93, in set_up_all
    self.verify(ret.lower() == self.dest, "MAC address wrong")
  File "/home/jenkins/dts-func/framework/test_case.py", line 169, in verify
    raise VerifyFailure(description)
exception.VerifyFailure: 'MAC address wrong'

06/05/2021 14:03:20                            dts:
TEST SUITE ENDED: TestDynamicConfig

The DTS code does not seem to have changed recently.
Is this test stable?


-- 
David Marchand

Re: [dpdk-dev] [PATCH 0/2] 21.05 fixes for OVS

[Aaron Conole]

David Marchand <david.marchand@redhat.com> writes:

> On Thu, May 6, 2021 at 5:15 PM David Marchand <david.marchand@redhat.com> wrote:
>>
>> Testing OVS build against 21.05-rc2 revealed some issues.
>> Bits were also missing on OVS side, so for people interested, a
>> rebased dpdk-latest OVS branch is available at:
>> https://github.com/david-marchand/ovs/commits/dpdk-latest

Thanks for the patches to OVS side - I'm currently reviewing them.

>
> I got failures for a "dynamic_config" job running on the Mellanox
> server from the lab.
> I don't see how my patches could break this.
>
> Max segment number per MTU/TSO: 40
> Switch name: 0000:05:00.0
> Switch domain Id: 0
> Switch Port Id: 65535
> 06/05/2021 14:03:20              TestDynamicConfig: set_up_all failed:
> Traceback (most recent call last):
>   File "/home/jenkins/dts-func/framework/test_case.py", line 258, in
> execute_setup_all
>     self.set_up_all()
>   File "tests/TestSuite_dynamic_config.py", line 93, in set_up_all
>     self.verify(ret.lower() == self.dest, "MAC address wrong")
>   File "/home/jenkins/dts-func/framework/test_case.py", line 169, in verify
>     raise VerifyFailure(description)
> exception.VerifyFailure: 'MAC address wrong'
>
> 06/05/2021 14:03:20                            dts:
> TEST SUITE ENDED: TestDynamicConfig
>
> The DTS code does not seem to have changed recently.
> Is this test stable?

Re: [dpdk-dev] [PATCH 0/2] 21.05 fixes for OVS

[David Marchand]

On Thu, May 6, 2021 at 5:15 PM David Marchand <david.marchand@redhat.com> wrote:
>
> Testing OVS build against 21.05-rc2 revealed some issues.
> Bits were also missing on OVS side, so for people interested, a
> rebased dpdk-latest OVS branch is available at:
> https://github.com/david-marchand/ovs/commits/dpdk-latest

Series applied, thanks.


-- 
David Marchand