| Message ID | 20220510142202.1087967-1-emma.finn@intel.com |
|---|---|
| Headers | show |
| Series | Actions Infrastructure + Optimizations | expand |
On 10 May 2022, at 16:21, Emma Finn wrote: > This patchset introduces actions infrastructure changes which allows the > user to choose between different action implementations based on CPU ISA > by using different commands. The infrastructure also provides a way to > check the correctness of the ISA optimized action version against the > scalar version. > > This series also introduces optimized versions of the following actions: > - push_vlan > - pop_vlan > - set_masked eth > - set_masked ipv4 Hi Emma, Just to let you know, I’ve started to review your series, and I did some performance testing on a none AVX512 machine, and the numbers look good, i.e., all within the standard division of my PVP tests. I’m trying to get my hands on an AVX512 machine and will do some more testing. In the meantime, I'll go over your changes, and I hope to get this done by the end of next week. Hopefully, I also have an AVX512 machine and get a chance to run some of the actual AVX code :) //Eelco
On 27 May 2022, at 14:55, Eelco Chaudron wrote: > On 10 May 2022, at 16:21, Emma Finn wrote: > >> This patchset introduces actions infrastructure changes which allows the >> user to choose between different action implementations based on CPU ISA >> by using different commands. The infrastructure also provides a way to >> check the correctness of the ISA optimized action version against the >> scalar version. >> >> This series also introduces optimized versions of the following actions: >> - push_vlan >> - pop_vlan >> - set_masked eth >> - set_masked ipv4 > > Hi Emma, > > Just to let you know, I’ve started to review your series, and I did some performance testing on a none AVX512 machine, and the numbers look good, i.e., all within the standard division of my PVP tests. I’m trying to get my hands on an AVX512 machine and will do some more testing. > > In the meantime, I'll go over your changes, and I hope to get this done by the end of next week. Hopefully, I also have an AVX512 machine and get a chance to run some of the actual AVX code :) I forgot to add that I decided not to go over the five revisions of review history, so if I bring up something that was discussed before, please point me to the previous discussion/conclusion. > //Eelco
This patchset introduces actions infrastructure changes which allows the
user to choose between different action implementations based on CPU ISA
by using different commands. The infrastructure also provides a way to
check the correctness of the ISA optimized action version against the
scalar version.
This series also introduces optimized versions of the following
actions:
- push_vlan
- pop_vlan
- set_masked eth
- set_masked ipv4
Below is a table indicating the relative performance benefits for these
actions.
+-----------------------------------------------+-------------------+-----------------+
| Actions | Salar with series |AVX with series |
+-----------------------------------------------+-------------------+-----------------+
| mod_dl_dst | 1.04x |1.15x |
+-----------------------------------------------+-------------------+-----------------+
| push_vlan | 1.10x |1.23x |
+-----------------------------------------------+-------------------+-----------------+
| strip_vlan | 1.05x |1.14x |
+-----------------------------------------------+-------------------+-----------------+
| mod_ipv4 1 x field | 1.04x |1.04x |
+-----------------------------------------------+-------------------+-----------------+
| mod_ipv4 4 x fields | 1.04x |1.23x |
+-----------------------------------------------+-------------------+-----------------+
| strip_vlan + mod_dl_dst + mod_ipv4 4 x fields | 1.06x |1.36x |
+-----------------------------------------------+-------------------+-----------------+
---
v7:
- Fix review comments from Eelco.
---
v6:
- Rebase to master
- Add ISA implementation of set_masked eth and ipv4 actions
- Fix incorrect checksums in input packets for ofproto-dpif unit tests
---
v5:
- Rebase to master
- Minor change to variable names
- Added Tags from Harry.
---
v4:
- Rebase to master
- Add ISA implementation of push_vlan action
---
v3:
- Refactored to fix unit test failures
- Removed some sign-off on commits
---
v2:
- Fix the CI build issues
---
Emma Finn (10):
ofproto-dpif: Fix incorrect checksums in input packets
odp-execute: Add function pointers to odp-execute for different action
implementations.
odp-execute: Add function pointer for pop_vlan action.
odp-execute: Add auto validation function for actions.
odp-execute: Add command to switch action implementation.
odp-execute: Add ISA implementation of actions.
odp-execute: Add ISA implementation of pop_vlan action.
odp-execute: Add ISA implementation of push_vlan action.
odp-execute: Add ISA implementation of set_masked ETH
odp-execute: Add ISA implementation of set_masked IPv4 action
Kumar Amber (1):
dpif-netdev: Add configure option to enable actions autovalidator at
build time.
Documentation/ref/ovs-actions.7.rst | 26 ++
Documentation/topics/testing.rst | 24 +-
NEWS | 11 +
acinclude.m4 | 21 ++
configure.ac | 1 +
lib/automake.mk | 8 +-
lib/cpu.c | 1 +
lib/cpu.h | 1 +
lib/dp-packet.c | 23 ++
lib/dp-packet.h | 4 +
lib/dpif-netdev-unixctl.man | 8 +
lib/dpif-netdev.c | 42 +++
lib/odp-execute-avx512.c | 463 ++++++++++++++++++++++++++++
lib/odp-execute-private.c | 266 ++++++++++++++++
lib/odp-execute-private.h | 99 ++++++
lib/odp-execute.c | 183 ++++++++---
lib/odp-execute.h | 14 +
tests/ofproto-dpif.at | 10 +-
tests/pmd.at | 30 ++
19 files changed, 1183 insertions(+), 52 deletions(-)
create mode 100644 lib/odp-execute-avx512.c
create mode 100644 lib/odp-execute-private.c
create mode 100644 lib/odp-execute-private.h
Hi Emma, I noticed this v7 while the discussion on the v6 has not yet finished. Maybe next time, it will be good to not send a new revision until the discussion on the previous revision has finished. This will potentially save another review round, as reviewing these large patchsets take a lot of time. However, I will review v7 and incorporate the v6 discussion. I’m rather busy right now, so I will try to get to this review in a week or two. Cheers, Eelco On 14 Jun 2022, at 13:57, Emma Finn wrote: > This patchset introduces actions infrastructure changes which allows the > user to choose between different action implementations based on CPU ISA > by using different commands. The infrastructure also provides a way to > check the correctness of the ISA optimized action version against the > scalar version. > > This series also introduces optimized versions of the following > actions: > - push_vlan > - pop_vlan > - set_masked eth > - set_masked ipv4 > > Below is a table indicating the relative performance benefits for these > actions. > > +-----------------------------------------------+-------------------+-----------------+ > | Actions | Salar with series |AVX with series | > +-----------------------------------------------+-------------------+-----------------+ > | mod_dl_dst | 1.04x |1.15x | > +-----------------------------------------------+-------------------+-----------------+ > | push_vlan | 1.10x |1.23x | > +-----------------------------------------------+-------------------+-----------------+ > | strip_vlan | 1.05x |1.14x | > +-----------------------------------------------+-------------------+-----------------+ > | mod_ipv4 1 x field | 1.04x |1.04x | > +-----------------------------------------------+-------------------+-----------------+ > | mod_ipv4 4 x fields | 1.04x |1.23x | > +-----------------------------------------------+-------------------+-----------------+ > | strip_vlan + mod_dl_dst + mod_ipv4 4 x fields | 1.06x |1.36x | > +-----------------------------------------------+-------------------+-----------------+ > > --- > v7: > - Fix review comments from Eelco. > --- > v6: > - Rebase to master > - Add ISA implementation of set_masked eth and ipv4 actions > - Fix incorrect checksums in input packets for ofproto-dpif unit tests > --- > v5: > - Rebase to master > - Minor change to variable names > - Added Tags from Harry. > --- > v4: > - Rebase to master > - Add ISA implementation of push_vlan action > --- > v3: > - Refactored to fix unit test failures > - Removed some sign-off on commits > --- > v2: > - Fix the CI build issues > --- > > Emma Finn (10): > ofproto-dpif: Fix incorrect checksums in input packets > odp-execute: Add function pointers to odp-execute for different action > implementations. > odp-execute: Add function pointer for pop_vlan action. > odp-execute: Add auto validation function for actions. > odp-execute: Add command to switch action implementation. > odp-execute: Add ISA implementation of actions. > odp-execute: Add ISA implementation of pop_vlan action. > odp-execute: Add ISA implementation of push_vlan action. > odp-execute: Add ISA implementation of set_masked ETH > odp-execute: Add ISA implementation of set_masked IPv4 action > > Kumar Amber (1): > dpif-netdev: Add configure option to enable actions autovalidator at > build time. > > Documentation/ref/ovs-actions.7.rst | 26 ++ > Documentation/topics/testing.rst | 24 +- > NEWS | 11 + > acinclude.m4 | 21 ++ > configure.ac | 1 + > lib/automake.mk | 8 +- > lib/cpu.c | 1 + > lib/cpu.h | 1 + > lib/dp-packet.c | 23 ++ > lib/dp-packet.h | 4 + > lib/dpif-netdev-unixctl.man | 8 + > lib/dpif-netdev.c | 42 +++ > lib/odp-execute-avx512.c | 463 ++++++++++++++++++++++++++++ > lib/odp-execute-private.c | 266 ++++++++++++++++ > lib/odp-execute-private.h | 99 ++++++ > lib/odp-execute.c | 183 ++++++++--- > lib/odp-execute.h | 14 + > tests/ofproto-dpif.at | 10 +- > tests/pmd.at | 30 ++ > 19 files changed, 1183 insertions(+), 52 deletions(-) > create mode 100644 lib/odp-execute-avx512.c > create mode 100644 lib/odp-execute-private.c > create mode 100644 lib/odp-execute-private.h > > -- > 2.32.0
Hi Emma,
I started reviewing your patch and found some issues that need investigation.
With the autovalidator, some tests are failing. But before you can run them, you need to fix your patch 7, as the autovalidator enablement is not working:
[wsfd-advnetlab44:~/...DK_v21.11/ovs_github]$ git diff
diff --git a/acinclude.m4 b/acinclude.m4
index 98f4599b1..18e61b522 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -28,7 +28,7 @@ AC_DEFUN([OVS_CHECK_ACTIONS_AUTOVALIDATOR], [
if test "$autovalidator" != yes; then
AC_MSG_RESULT([no])
else
- AC_DEFINE([MFEX_AUTOVALIDATOR_DEFAULT], [1],
+ AC_DEFINE([ACTIONS_AUTOVALIDATOR_DEFAULT], [1],
[Autovalidator for actions is a default implementation.])
AC_MSG_RESULT([yes])
fi
Build OVS (without DPDK) and run the following tests:
# ./configure --prefix=/usr --localstatedir=/var --sysconfdir=/etc && make -j 32 check TESTSUITEFLAGS="1040 1041 1132 1145 1294 2493"
...
...
## ------------------------------- ##
## openvswitch 2.17.90 test suite. ##
## ------------------------------- ##
dpif-netdev
1040: dpif-netdev - partial hw offload with packet modifications - dummy ok
1041: dpif-netdev - partial hw offload with packet modifications - dummy-pmd ok
ofproto-dpif
1132: ofproto-dpif - controller ok
1145: ofproto-dpif - ARP modification slow-path ok
ofproto-dpif - flow translation resource limits
1294: ofproto-dpif - Neighbor Discovery set-field with checksum update ok
network service header (NSH)
2493: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe ok
## ------------- ##
## Test results. ##
## ------------- ##
All 6 tests were successful.
Now with the auto-validator on an AVX512 machine:
# ./configure --prefix=/usr --localstatedir=/var --sysconfdir=/etc --enable-actions-default-autovalidator && make -j 32 check TESTSUITEFLAGS="1040 1041 1132 1145 1294 2493"
...
...
## ------------------------------- ##
## openvswitch 2.17.90 test suite. ##
## ------------------------------- ##
dpif-netdev
1040: dpif-netdev - partial hw offload with packet modifications - dummy FAILED (dpif-netdev.at:542)
1041: dpif-netdev - partial hw offload with packet modifications - dummy-pmd FAILED (dpif-netdev.at:543)
ofproto-dpif
1132: ofproto-dpif - controller FAILED (ofproto-dpif.at:1979)
1145: ofproto-dpif - ARP modification slow-path FAILED (ofproto-dpif.at:3777)
ofproto-dpif - flow translation resource limits
1294: ofproto-dpif - Neighbor Discovery set-field with checksum update FAILED (ofproto-dpif.at:9936)
network service header (NSH)
2493: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED (nsh.at:778)
## ------------- ##
## Test results. ##
## ------------- ##
ERROR: All 6 tests were run,
6 failed unexpectedly.
Please start some investigation, but do NOT send out a v8, until I’ve completed v7. If there is a small change I can apply that fixes the issue, just sent that in this thread.
//Eelco
On 14 Jun 2022, at 13:57, Emma Finn wrote:
> This patchset introduces actions infrastructure changes which allows the
> user to choose between different action implementations based on CPU ISA
> by using different commands. The infrastructure also provides a way to
> check the correctness of the ISA optimized action version against the
> scalar version.
>
> This series also introduces optimized versions of the following
> actions:
> - push_vlan
> - pop_vlan
> - set_masked eth
> - set_masked ipv4
>
> Below is a table indicating the relative performance benefits for these
> actions.
>
> +-----------------------------------------------+-------------------+-----------------+
> | Actions | Salar with series |AVX with series |
> +-----------------------------------------------+-------------------+-----------------+
> | mod_dl_dst | 1.04x |1.15x |
> +-----------------------------------------------+-------------------+-----------------+
> | push_vlan | 1.10x |1.23x |
> +-----------------------------------------------+-------------------+-----------------+
> | strip_vlan | 1.05x |1.14x |
> +-----------------------------------------------+-------------------+-----------------+
> | mod_ipv4 1 x field | 1.04x |1.04x |
> +-----------------------------------------------+-------------------+-----------------+
> | mod_ipv4 4 x fields | 1.04x |1.23x |
> +-----------------------------------------------+-------------------+-----------------+
> | strip_vlan + mod_dl_dst + mod_ipv4 4 x fields | 1.06x |1.36x |
> +-----------------------------------------------+-------------------+-----------------+
>
> ---
> v7:
> - Fix review comments from Eelco.
> ---
> v6:
> - Rebase to master
> - Add ISA implementation of set_masked eth and ipv4 actions
> - Fix incorrect checksums in input packets for ofproto-dpif unit tests
> ---
> v5:
> - Rebase to master
> - Minor change to variable names
> - Added Tags from Harry.
> ---
> v4:
> - Rebase to master
> - Add ISA implementation of push_vlan action
> ---
> v3:
> - Refactored to fix unit test failures
> - Removed some sign-off on commits
> ---
> v2:
> - Fix the CI build issues
> ---
>
> Emma Finn (10):
> ofproto-dpif: Fix incorrect checksums in input packets
> odp-execute: Add function pointers to odp-execute for different action
> implementations.
> odp-execute: Add function pointer for pop_vlan action.
> odp-execute: Add auto validation function for actions.
> odp-execute: Add command to switch action implementation.
> odp-execute: Add ISA implementation of actions.
> odp-execute: Add ISA implementation of pop_vlan action.
> odp-execute: Add ISA implementation of push_vlan action.
> odp-execute: Add ISA implementation of set_masked ETH
> odp-execute: Add ISA implementation of set_masked IPv4 action
>
> Kumar Amber (1):
> dpif-netdev: Add configure option to enable actions autovalidator at
> build time.
>
> Documentation/ref/ovs-actions.7.rst | 26 ++
> Documentation/topics/testing.rst | 24 +-
> NEWS | 11 +
> acinclude.m4 | 21 ++
> configure.ac | 1 +
> lib/automake.mk | 8 +-
> lib/cpu.c | 1 +
> lib/cpu.h | 1 +
> lib/dp-packet.c | 23 ++
> lib/dp-packet.h | 4 +
> lib/dpif-netdev-unixctl.man | 8 +
> lib/dpif-netdev.c | 42 +++
> lib/odp-execute-avx512.c | 463 ++++++++++++++++++++++++++++
> lib/odp-execute-private.c | 266 ++++++++++++++++
> lib/odp-execute-private.h | 99 ++++++
> lib/odp-execute.c | 183 ++++++++---
> lib/odp-execute.h | 14 +
> tests/ofproto-dpif.at | 10 +-
> tests/pmd.at | 30 ++
> 19 files changed, 1183 insertions(+), 52 deletions(-)
> create mode 100644 lib/odp-execute-avx512.c
> create mode 100644 lib/odp-execute-private.c
> create mode 100644 lib/odp-execute-private.h
>
> --
> 2.32.0
Hi Eelco,
I've investigated the issue and resolved the failing unit tests.
Diff's are provided below or if it's convenient I can send as patchset instead.
Fix for 05/11: odp-execute: Add command to switch action implementation.
diff --git a/tests/pmd.at b/tests/pmd.at
index ac05f5f7d..140ce8a8d 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -1205,8 +1205,12 @@ AT_SETUP([PMD - ovs-actions configuration])
OVS_VSWITCHD_START([], [], [], [--dummy-numa 0,0])
AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy-pmd])
-dnl Scalar impl is set by default.
AT_CHECK([ovs-vsctl show], [], [stdout])
+
+dnl Set the scalar first, so we always have the scalar impl as Active.
+AT_CHECK([ovs-appctl dpif-netdev/action-impl-set scalar], [0], [dnl
+Action implementation set to scalar.
+])
AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "scalar"], [], [dnl
scalar (available: Yes, active: Yes)
])
Fix for 06/11 dpif-netdev: Add configure option to enable actions autovalidator at build time.
diff --git a/acinclude.m4 b/acinclude.m4
index 98f4599b1..18e61b522 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -28,7 +28,7 @@ AC_DEFUN([OVS_CHECK_ACTIONS_AUTOVALIDATOR], [
if test "$autovalidator" != yes; then
AC_MSG_RESULT([no])
else
- AC_DEFINE([MFEX_AUTOVALIDATOR_DEFAULT], [1],
+ AC_DEFINE([ACTIONS_AUTOVALIDATOR_DEFAULT], [1],
[Autovalidator for actions is a default implementation.])
AC_MSG_RESULT([yes])
Fi
Fix for 10/11 odp-execute: Add ISA implementation of set_masked ETH
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index e2d650779..6c3dabbd0 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -21,6 +21,7 @@
#include "dpdk.h"
#include "dp-packet.h"
+#include "odp-execute.h"
#include "odp-execute-private.h"
#include "odp-netlink.h"
#include "odp-util.h"
@@ -243,6 +244,13 @@ action_set_masked_init(struct dp_packet_batch *batch OVS_UNUSED,
if (autoval_impl.set_masked_funcs[attr_type]) {
set_masked = true;
autoval_impl.set_masked_funcs[attr_type](batch, a);
+ } else {
+ struct dp_packet *packet;
+ a = nl_attr_get(a);
+
+ DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
+ odp_execute_masked_set_action(packet, a);
+ }
}
}
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index db6e1ec03..2aa213399 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -561,7 +561,7 @@ odp_execute_set_action(struct dp_packet *packet, const struct nlattr *a)
}
}
-static void
+void
odp_execute_masked_set_action(struct dp_packet *packet,
const struct nlattr *a)
diff --git a/lib/odp-execute.h b/lib/odp-execute.h
index 762b99473..4857cb91f 100644
--- a/lib/odp-execute.h
+++ b/lib/odp-execute.h
@@ -53,4 +53,7 @@ void odp_execute_actions(void *dp, struct dp_packet_batch *batch,
#define get_mask(a, type) ((const type *)(const void *)(a + 1) + 1)
+void odp_execute_masked_set_action(struct dp_packet *packet,
+ const struct nlattr *a);
+
#endif
diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
index aa65afec7..22a96b1c8 100644
--- a/lib/odp-execute-avx512.c
+++ b/lib/odp-execute-avx512.c
@@ -26,6 +26,7 @@
#include "cpu.h"
#include "dp-packet.h"
#include "immintrin.h"
+#include "odp-execute.h"
#include "odp-execute-private.h"
#include "odp-netlink.h"
#include "openvswitch/vlog.h"
@@ -411,6 +412,13 @@ action_avx512_set_masked(struct dp_packet_batch *batch OVS_UNUSED,
if (avx512_impl.set_masked_funcs[attr_type]) {
avx512_impl.set_masked_funcs[attr_type](batch, a);
+ } else {
+ struct dp_packet *packet;
+ a = nl_attr_get(a);
+
+ DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
+ odp_execute_masked_set_action(packet, a);
+ }
}
diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
index aa2ed9022..6431b49dc 100644
--- a/lib/odp-execute-avx512.c
+++ b/lib/odp-execute-avx512.c
@@ -197,8 +197,8 @@ static void
action_avx512_set_masked(struct dp_packet_batch *batch OVS_UNUSED,
const struct nlattr *a)
{
- a = nl_attr_get(a);
- enum ovs_key_attr attr_type = nl_attr_type(a);
+ const struct nlattr *type = nl_attr_get(a);
+ enum ovs_key_attr attr_type = nl_attr_type(type);
Thanks,
Emma
> -----Original Message-----
> From: Eelco Chaudron <echaudro@redhat.com>
> Sent: Tuesday 21 June 2022 14:29
> To: Finn, Emma <emma.finn@intel.com>
> Cc: Stokes, Ian <ian.stokes@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>; dev@openvswitch.org
> Subject: Re: [PATCH v7 00/11] Actions Infrastructure + Optimizations
>
> Hi Emma,
>
> I started reviewing your patch and found some issues that need investigation.
>
> With the autovalidator, some tests are failing. But before you can run them, you
> need to fix your patch 7, as the autovalidator enablement is not working:
>
> [wsfd-advnetlab44:~/...DK_v21.11/ovs_github]$ git diff diff --git a/acinclude.m4
> b/acinclude.m4 index 98f4599b1..18e61b522 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -28,7 +28,7 @@ AC_DEFUN([OVS_CHECK_ACTIONS_AUTOVALIDATOR], [
> if test "$autovalidator" != yes; then
> AC_MSG_RESULT([no])
> else
> - AC_DEFINE([MFEX_AUTOVALIDATOR_DEFAULT], [1],
> + AC_DEFINE([ACTIONS_AUTOVALIDATOR_DEFAULT], [1],
> [Autovalidator for actions is a default implementation.])
> AC_MSG_RESULT([yes])
> fi
>
>
> Build OVS (without DPDK) and run the following tests:
>
> # ./configure --prefix=/usr --localstatedir=/var --sysconfdir=/etc && make -j 32
> check TESTSUITEFLAGS="1040 1041 1132 1145 1294 2493"
> ...
> ...
> ## ------------------------------- ##
> ## openvswitch 2.17.90 test suite. ##
> ## ------------------------------- ##
>
> dpif-netdev
>
> 1040: dpif-netdev - partial hw offload with packet modifications - dummy ok
> 1041: dpif-netdev - partial hw offload with packet modifications - dummy-pmd
> ok
>
> ofproto-dpif
>
> 1132: ofproto-dpif - controller ok
> 1145: ofproto-dpif - ARP modification slow-path ok
>
> ofproto-dpif - flow translation resource limits
>
> 1294: ofproto-dpif - Neighbor Discovery set-field with checksum update ok
>
> network service header (NSH)
>
> 2493: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe ok
>
> ## ------------- ##
> ## Test results. ##
> ## ------------- ##
>
> All 6 tests were successful.
>
>
> Now with the auto-validator on an AVX512 machine:
>
> # ./configure --prefix=/usr --localstatedir=/var --sysconfdir=/etc --enable-
> actions-default-autovalidator && make -j 32 check TESTSUITEFLAGS="1040
> 1041 1132 1145 1294 2493"
> ...
> ...
> ## ------------------------------- ##
> ## openvswitch 2.17.90 test suite. ##
> ## ------------------------------- ##
>
> dpif-netdev
>
> 1040: dpif-netdev - partial hw offload with packet modifications - dummy
> FAILED (dpif-netdev.at:542)
> 1041: dpif-netdev - partial hw offload with packet modifications - dummy-pmd
> FAILED (dpif-netdev.at:543)
>
> ofproto-dpif
>
> 1132: ofproto-dpif - controller FAILED (ofproto-dpif.at:1979)
> 1145: ofproto-dpif - ARP modification slow-path FAILED (ofproto-
> dpif.at:3777)
>
> ofproto-dpif - flow translation resource limits
>
> 1294: ofproto-dpif - Neighbor Discovery set-field with checksum update FAILED
> (ofproto-dpif.at:9936)
>
> network service header (NSH)
>
> 2493: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED
> (nsh.at:778)
>
> ## ------------- ##
> ## Test results. ##
> ## ------------- ##
>
> ERROR: All 6 tests were run,
> 6 failed unexpectedly.
>
>
> Please start some investigation, but do NOT send out a v8, until I’ve completed
> v7. If there is a small change I can apply that fixes the issue, just sent that in this
> thread.
>
>
> //Eelco
>
> On 14 Jun 2022, at 13:57, Emma Finn wrote:
>
> > This patchset introduces actions infrastructure changes which allows
> > the user to choose between different action implementations based on
> > CPU ISA by using different commands. The infrastructure also provides
> > a way to check the correctness of the ISA optimized action version
> > against the scalar version.
> >
> > This series also introduces optimized versions of the following
> > actions:
> > - push_vlan
> > - pop_vlan
> > - set_masked eth
> > - set_masked ipv4
> >
> > Below is a table indicating the relative performance benefits for
> > these actions.
> >
> > +-----------------------------------------------+-------------------+-----------------+
> > | Actions | Salar with series |AVX with series |
> > +-----------------------------------------------+-------------------+-----------------+
> > | mod_dl_dst | 1.04x |1.15x |
> > +-----------------------------------------------+-------------------+-----------------+
> > | push_vlan | 1.10x |1.23x |
> > +-----------------------------------------------+-------------------+-----------------+
> > | strip_vlan | 1.05x |1.14x |
> > +-----------------------------------------------+-------------------+-----------------+
> > | mod_ipv4 1 x field | 1.04x |1.04x |
> > +-----------------------------------------------+-------------------+-----------------+
> > | mod_ipv4 4 x fields | 1.04x |1.23x |
> > +-----------------------------------------------+-------------------+-----------------+
> > | strip_vlan + mod_dl_dst + mod_ipv4 4 x fields | 1.06x |1.36x |
> > +-----------------------------------------------+-------------------+-----------------+
> >
> > ---
> > v7:
> > - Fix review comments from Eelco.
> > ---
> > v6:
> > - Rebase to master
> > - Add ISA implementation of set_masked eth and ipv4 actions
> > - Fix incorrect checksums in input packets for ofproto-dpif unit tests
> > ---
> > v5:
> > - Rebase to master
> > - Minor change to variable names
> > - Added Tags from Harry.
> > ---
> > v4:
> > - Rebase to master
> > - Add ISA implementation of push_vlan action
> > ---
> > v3:
> > - Refactored to fix unit test failures
> > - Removed some sign-off on commits
> > ---
> > v2:
> > - Fix the CI build issues
> > ---
> >
> > Emma Finn (10):
> > ofproto-dpif: Fix incorrect checksums in input packets
> > odp-execute: Add function pointers to odp-execute for different action
> > implementations.
> > odp-execute: Add function pointer for pop_vlan action.
> > odp-execute: Add auto validation function for actions.
> > odp-execute: Add command to switch action implementation.
> > odp-execute: Add ISA implementation of actions.
> > odp-execute: Add ISA implementation of pop_vlan action.
> > odp-execute: Add ISA implementation of push_vlan action.
> > odp-execute: Add ISA implementation of set_masked ETH
> > odp-execute: Add ISA implementation of set_masked IPv4 action
> >
> > Kumar Amber (1):
> > dpif-netdev: Add configure option to enable actions autovalidator at
> > build time.
> >
> > Documentation/ref/ovs-actions.7.rst | 26 ++
> > Documentation/topics/testing.rst | 24 +-
> > NEWS | 11 +
> > acinclude.m4 | 21 ++
> > configure.ac | 1 +
> > lib/automake.mk | 8 +-
> > lib/cpu.c | 1 +
> > lib/cpu.h | 1 +
> > lib/dp-packet.c | 23 ++
> > lib/dp-packet.h | 4 +
> > lib/dpif-netdev-unixctl.man | 8 +
> > lib/dpif-netdev.c | 42 +++
> > lib/odp-execute-avx512.c | 463 ++++++++++++++++++++++++++++
> > lib/odp-execute-private.c | 266 ++++++++++++++++
> > lib/odp-execute-private.h | 99 ++++++
> > lib/odp-execute.c | 183 ++++++++---
> > lib/odp-execute.h | 14 +
> > tests/ofproto-dpif.at | 10 +-
> > tests/pmd.at | 30 ++
> > 19 files changed, 1183 insertions(+), 52 deletions(-) create mode
> > 100644 lib/odp-execute-avx512.c create mode 100644
> > lib/odp-execute-private.c create mode 100644
> > lib/odp-execute-private.h
> >
> > --
> > 2.32.0
On 22 Jun 2022, at 18:22, Finn, Emma wrote: > Hi Eelco, > > I've investigated the issue and resolved the failing unit tests. > Diff's are provided below or if it's convenient I can send as patchset instead. > > Fix for 05/11: odp-execute: Add command to switch action implementation. > > diff --git a/tests/pmd.at b/tests/pmd.at > index ac05f5f7d..140ce8a8d 100644 > --- a/tests/pmd.at > +++ b/tests/pmd.at > @@ -1205,8 +1205,12 @@ AT_SETUP([PMD - ovs-actions configuration]) > OVS_VSWITCHD_START([], [], [], [--dummy-numa 0,0]) > AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy-pmd]) > > -dnl Scalar impl is set by default. > AT_CHECK([ovs-vsctl show], [], [stdout]) > + > +dnl Set the scalar first, so we always have the scalar impl as Active. > +AT_CHECK([ovs-appctl dpif-netdev/action-impl-set scalar], [0], [dnl > +Action implementation set to scalar. > +]) > AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "scalar"], [], [dnl > scalar (available: Yes, active: Yes) > ]) > > > Fix for 06/11 dpif-netdev: Add configure option to enable actions autovalidator at build time. > > diff --git a/acinclude.m4 b/acinclude.m4 > index 98f4599b1..18e61b522 100644 > --- a/acinclude.m4 > +++ b/acinclude.m4 > @@ -28,7 +28,7 @@ AC_DEFUN([OVS_CHECK_ACTIONS_AUTOVALIDATOR], [ > if test "$autovalidator" != yes; then > AC_MSG_RESULT([no]) > else > - AC_DEFINE([MFEX_AUTOVALIDATOR_DEFAULT], [1], > + AC_DEFINE([ACTIONS_AUTOVALIDATOR_DEFAULT], [1], > [Autovalidator for actions is a default implementation.]) > AC_MSG_RESULT([yes]) > Fi Hi Emma, Thanks for looking into this! The two changes above I already identified as part of my review in progress (I’m currently starting with patch 11). > Fix for 10/11 odp-execute: Add ISA implementation of set_masked ETH > > diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c > index e2d650779..6c3dabbd0 100644 > --- a/lib/odp-execute-private.c > +++ b/lib/odp-execute-private.c > @@ -21,6 +21,7 @@ > > #include "dpdk.h" > #include "dp-packet.h" > +#include "odp-execute.h" > #include "odp-execute-private.h" > #include "odp-netlink.h" > #include "odp-util.h" > @@ -243,6 +244,13 @@ action_set_masked_init(struct dp_packet_batch *batch OVS_UNUSED, > if (autoval_impl.set_masked_funcs[attr_type]) { > set_masked = true; > autoval_impl.set_masked_funcs[attr_type](batch, a); > + } else { > + struct dp_packet *packet; > + a = nl_attr_get(a); > + > + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { > + odp_execute_masked_set_action(packet, a); > + } > } > } > > diff --git a/lib/odp-execute.c b/lib/odp-execute.c > index db6e1ec03..2aa213399 100644 > --- a/lib/odp-execute.c > +++ b/lib/odp-execute.c > @@ -561,7 +561,7 @@ odp_execute_set_action(struct dp_packet *packet, const struct nlattr *a) > } > } > > -static void > +void > odp_execute_masked_set_action(struct dp_packet *packet, > const struct nlattr *a) > > diff --git a/lib/odp-execute.h b/lib/odp-execute.h > index 762b99473..4857cb91f 100644 > --- a/lib/odp-execute.h > +++ b/lib/odp-execute.h > @@ -53,4 +53,7 @@ void odp_execute_actions(void *dp, struct dp_packet_batch *batch, > > #define get_mask(a, type) ((const type *)(const void *)(a + 1) + 1) > > +void odp_execute_masked_set_action(struct dp_packet *packet, > + const struct nlattr *a); > + > #endif > > diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c > index aa65afec7..22a96b1c8 100644 > --- a/lib/odp-execute-avx512.c > +++ b/lib/odp-execute-avx512.c > @@ -26,6 +26,7 @@ > #include "cpu.h" > #include "dp-packet.h" > #include "immintrin.h" > +#include "odp-execute.h" > #include "odp-execute-private.h" > #include "odp-netlink.h" > #include "openvswitch/vlog.h" > @@ -411,6 +412,13 @@ action_avx512_set_masked(struct dp_packet_batch *batch OVS_UNUSED, > > if (avx512_impl.set_masked_funcs[attr_type]) { > avx512_impl.set_masked_funcs[attr_type](batch, a); > + } else { > + struct dp_packet *packet; > + a = nl_attr_get(a); > + > + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { > + odp_execute_masked_set_action(packet, a); > + } > } > > diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c > index aa2ed9022..6431b49dc 100644 > --- a/lib/odp-execute-avx512.c > +++ b/lib/odp-execute-avx512.c > @@ -197,8 +197,8 @@ static void > action_avx512_set_masked(struct dp_packet_batch *batch OVS_UNUSED, > const struct nlattr *a) > { > - a = nl_attr_get(a); > - enum ovs_key_attr attr_type = nl_attr_type(a); > + const struct nlattr *type = nl_attr_get(a); > + enum ovs_key_attr attr_type = nl_attr_type(type); I re-wrote most of patch 10, and looking at the changes, they might no longer be needed. However, I still need to test all my suggestions :) So no patch update is needed for now, and I’ll make sure it’s all covered as part of my review feedback. Cheers, Eelco > Thanks, > Emma > >> -----Original Message----- >> From: Eelco Chaudron <echaudro@redhat.com> >> Sent: Tuesday 21 June 2022 14:29 >> To: Finn, Emma <emma.finn@intel.com> >> Cc: Stokes, Ian <ian.stokes@intel.com>; Van Haaren, Harry >> <harry.van.haaren@intel.com>; dev@openvswitch.org >> Subject: Re: [PATCH v7 00/11] Actions Infrastructure + Optimizations >> >> Hi Emma, >> >> I started reviewing your patch and found some issues that need investigation. >> >> With the autovalidator, some tests are failing. But before you can run them, you >> need to fix your patch 7, as the autovalidator enablement is not working: >> >> [wsfd-advnetlab44:~/...DK_v21.11/ovs_github]$ git diff diff --git a/acinclude.m4 >> b/acinclude.m4 index 98f4599b1..18e61b522 100644 >> --- a/acinclude.m4 >> +++ b/acinclude.m4 >> @@ -28,7 +28,7 @@ AC_DEFUN([OVS_CHECK_ACTIONS_AUTOVALIDATOR], [ >> if test "$autovalidator" != yes; then >> AC_MSG_RESULT([no]) >> else >> - AC_DEFINE([MFEX_AUTOVALIDATOR_DEFAULT], [1], >> + AC_DEFINE([ACTIONS_AUTOVALIDATOR_DEFAULT], [1], >> [Autovalidator for actions is a default implementation.]) >> AC_MSG_RESULT([yes]) >> fi >> >> >> Build OVS (without DPDK) and run the following tests: >> >> # ./configure --prefix=/usr --localstatedir=/var --sysconfdir=/etc && make -j 32 >> check TESTSUITEFLAGS="1040 1041 1132 1145 1294 2493" >> ... >> ... >> ## ------------------------------- ## >> ## openvswitch 2.17.90 test suite. ## >> ## ------------------------------- ## >> >> dpif-netdev >> >> 1040: dpif-netdev - partial hw offload with packet modifications - dummy ok >> 1041: dpif-netdev - partial hw offload with packet modifications - dummy-pmd >> ok >> >> ofproto-dpif >> >> 1132: ofproto-dpif - controller ok >> 1145: ofproto-dpif - ARP modification slow-path ok >> >> ofproto-dpif - flow translation resource limits >> >> 1294: ofproto-dpif - Neighbor Discovery set-field with checksum update ok >> >> network service header (NSH) >> >> 2493: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe ok >> >> ## ------------- ## >> ## Test results. ## >> ## ------------- ## >> >> All 6 tests were successful. >> >> >> Now with the auto-validator on an AVX512 machine: >> >> # ./configure --prefix=/usr --localstatedir=/var --sysconfdir=/etc --enable- >> actions-default-autovalidator && make -j 32 check TESTSUITEFLAGS="1040 >> 1041 1132 1145 1294 2493" >> ... >> ... >> ## ------------------------------- ## >> ## openvswitch 2.17.90 test suite. ## >> ## ------------------------------- ## >> >> dpif-netdev >> >> 1040: dpif-netdev - partial hw offload with packet modifications - dummy >> FAILED (dpif-netdev.at:542) >> 1041: dpif-netdev - partial hw offload with packet modifications - dummy-pmd >> FAILED (dpif-netdev.at:543) >> >> ofproto-dpif >> >> 1132: ofproto-dpif - controller FAILED (ofproto-dpif.at:1979) >> 1145: ofproto-dpif - ARP modification slow-path FAILED (ofproto- >> dpif.at:3777) >> >> ofproto-dpif - flow translation resource limits >> >> 1294: ofproto-dpif - Neighbor Discovery set-field with checksum update FAILED >> (ofproto-dpif.at:9936) >> >> network service header (NSH) >> >> 2493: nsh - triangle PTAP bridge setup with NSH over vxlan-gpe FAILED >> (nsh.at:778) >> >> ## ------------- ## >> ## Test results. ## >> ## ------------- ## >> >> ERROR: All 6 tests were run, >> 6 failed unexpectedly. >> >> >> Please start some investigation, but do NOT send out a v8, until I’ve completed >> v7. If there is a small change I can apply that fixes the issue, just sent that in this >> thread. >> >> >> //Eelco >> >> On 14 Jun 2022, at 13:57, Emma Finn wrote: >> >>> This patchset introduces actions infrastructure changes which allows >>> the user to choose between different action implementations based on >>> CPU ISA by using different commands. The infrastructure also provides >>> a way to check the correctness of the ISA optimized action version >>> against the scalar version. >>> >>> This series also introduces optimized versions of the following >>> actions: >>> - push_vlan >>> - pop_vlan >>> - set_masked eth >>> - set_masked ipv4 >>> >>> Below is a table indicating the relative performance benefits for >>> these actions. >>> >>> +-----------------------------------------------+-------------------+-----------------+ >>> | Actions | Salar with series |AVX with series | >>> +-----------------------------------------------+-------------------+-----------------+ >>> | mod_dl_dst | 1.04x |1.15x | >>> +-----------------------------------------------+-------------------+-----------------+ >>> | push_vlan | 1.10x |1.23x | >>> +-----------------------------------------------+-------------------+-----------------+ >>> | strip_vlan | 1.05x |1.14x | >>> +-----------------------------------------------+-------------------+-----------------+ >>> | mod_ipv4 1 x field | 1.04x |1.04x | >>> +-----------------------------------------------+-------------------+-----------------+ >>> | mod_ipv4 4 x fields | 1.04x |1.23x | >>> +-----------------------------------------------+-------------------+-----------------+ >>> | strip_vlan + mod_dl_dst + mod_ipv4 4 x fields | 1.06x |1.36x | >>> +-----------------------------------------------+-------------------+-----------------+ >>> >>> --- >>> v7: >>> - Fix review comments from Eelco. >>> --- >>> v6: >>> - Rebase to master >>> - Add ISA implementation of set_masked eth and ipv4 actions >>> - Fix incorrect checksums in input packets for ofproto-dpif unit tests >>> --- >>> v5: >>> - Rebase to master >>> - Minor change to variable names >>> - Added Tags from Harry. >>> --- >>> v4: >>> - Rebase to master >>> - Add ISA implementation of push_vlan action >>> --- >>> v3: >>> - Refactored to fix unit test failures >>> - Removed some sign-off on commits >>> --- >>> v2: >>> - Fix the CI build issues >>> --- >>> >>> Emma Finn (10): >>> ofproto-dpif: Fix incorrect checksums in input packets >>> odp-execute: Add function pointers to odp-execute for different action >>> implementations. >>> odp-execute: Add function pointer for pop_vlan action. >>> odp-execute: Add auto validation function for actions. >>> odp-execute: Add command to switch action implementation. >>> odp-execute: Add ISA implementation of actions. >>> odp-execute: Add ISA implementation of pop_vlan action. >>> odp-execute: Add ISA implementation of push_vlan action. >>> odp-execute: Add ISA implementation of set_masked ETH >>> odp-execute: Add ISA implementation of set_masked IPv4 action >>> >>> Kumar Amber (1): >>> dpif-netdev: Add configure option to enable actions autovalidator at >>> build time. >>> >>> Documentation/ref/ovs-actions.7.rst | 26 ++ >>> Documentation/topics/testing.rst | 24 +- >>> NEWS | 11 + >>> acinclude.m4 | 21 ++ >>> configure.ac | 1 + >>> lib/automake.mk | 8 +- >>> lib/cpu.c | 1 + >>> lib/cpu.h | 1 + >>> lib/dp-packet.c | 23 ++ >>> lib/dp-packet.h | 4 + >>> lib/dpif-netdev-unixctl.man | 8 + >>> lib/dpif-netdev.c | 42 +++ >>> lib/odp-execute-avx512.c | 463 ++++++++++++++++++++++++++++ >>> lib/odp-execute-private.c | 266 ++++++++++++++++ >>> lib/odp-execute-private.h | 99 ++++++ >>> lib/odp-execute.c | 183 ++++++++--- >>> lib/odp-execute.h | 14 + >>> tests/ofproto-dpif.at | 10 +- >>> tests/pmd.at | 30 ++ >>> 19 files changed, 1183 insertions(+), 52 deletions(-) create mode >>> 100644 lib/odp-execute-avx512.c create mode 100644 >>> lib/odp-execute-private.c create mode 100644 >>> lib/odp-execute-private.h >>> >>> -- >>> 2.32.0
On 14 Jun 2022, at 13:57, Emma Finn wrote: > This patchset introduces actions infrastructure changes which allows the > user to choose between different action implementations based on CPU ISA > by using different commands. The infrastructure also provides a way to > check the correctness of the ISA optimized action version against the > scalar version. > <SNIP> Hi Emma, I sent out my review comments yesterday but got a lot of bounce emails for the Intel recipients, so not sure if you got them. They seem to have arrived at the mailing list and patchwork: https://patchwork.ozlabs.org/project/openvswitch/list/?series=304718 Cheers, Eelco
This patchset introduces actions infrastructure changes which allows the user to choose between different action implementations based on CPU ISA by using different commands. The infrastructure also provides a way to check the correctness of the ISA optimized action version against the scalar version. This series also introduces optimized versions of the following actions: - push_vlan - pop_vlan - set_masked eth - set_masked ipv4 Below is a table indicating the relative performance benefits for these actions. +-----------------------------------------------+-------------------+-----------------+ | Actions | Salar with series | AVX with series | +-----------------------------------------------+-------------------+-----------------+ | mod_dl_dst | 1.04x | 1.15x | +-----------------------------------------------+-------------------+-----------------+ | push_vlan | 1.10x | 1.23x | +-----------------------------------------------+-------------------+-----------------+ | strip_vlan | 1.05x | 1.14x | +-----------------------------------------------+-------------------+-----------------+ | mod_ipv4 1 x field | 1.04x | 1.04x | +-----------------------------------------------+-------------------+-----------------+ | mod_ipv4 4 x fields | 1.04x | 1.23x | +-----------------------------------------------+-------------------+-----------------+ | strip_vlan + mod_dl_dst + mod_ipv4 4 x fields | 1.06x | 1.36x | +-----------------------------------------------+-------------------+-----------------+ --- v6: - Rebase to master - Add ISA implementation of set_masked eth and ipv4 actions - Fix incorrect checksums in input packets for ofproto-dpif unit tests --- v5: - Rebase to master - Minor change to variable names - Added Tags from Harry. --- v4: - Rebase to master - Add ISA implementation of push_vlan action --- v3: - Refactored to fix unit test failures - Removed some sign-off on commits --- v2: - Fix the CI build issues --- Emma Finn (10): ofproto-dpif: Fix incorrect checksums in input packets odp-execute: Add function pointers to odp-execute for different action implementations. odp-execute: Add function pointer for pop_vlan action. odp-execute: Add auto validation function for actions. odp-execute: Add command to switch action implementation. odp-execute: Add ISA implementation of actions. odp-execute: Add ISA implementation of pop_vlan action. odp-execute: Add ISA implementation of push_vlan action. odp-execute: Add ISA implementation of set_masked ETH action odp-execute: Add ISA implementation of set_masked IPv4 action Kumar Amber (1): dpif-netdev: Add configure option to enable actions autovalidator at build time. Documentation/topics/dpdk/bridge.rst | 25 + Documentation/topics/testing.rst | 20 +- NEWS | 7 + acinclude.m4 | 17 + configure.ac | 1 + .../linux/compat/include/linux/openvswitch.h | 2 +- lib/automake.mk | 6 +- lib/cpu.c | 1 + lib/cpu.h | 1 + lib/dp-packet.c | 23 + lib/dp-packet.h | 4 + lib/dpif-netdev-unixctl.man | 6 + lib/dpif-netdev.c | 43 ++ lib/odp-execute-avx512.c | 436 ++++++++++++++++++ lib/odp-execute-private.c | 291 ++++++++++++ lib/odp-execute-private.h | 109 +++++ lib/odp-execute.c | 186 ++++++-- lib/odp-execute.h | 11 + tests/ofproto-dpif.at | 10 +- tests/pmd.at | 21 + 20 files changed, 1168 insertions(+), 52 deletions(-) create mode 100644 lib/odp-execute-avx512.c create mode 100644 lib/odp-execute-private.c create mode 100644 lib/odp-execute-private.h