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 <[email protected]>
> Sent: Tuesday 21 June 2022 14:29
> To: Finn, Emma <[email protected]>
> Cc: Stokes, Ian <[email protected]>; Van Haaren, Harry
> <[email protected]>; [email protected]
> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
