diff mbox series

[ovs-dev,v8,06/10] odp-execute: Add ISA implementation of actions.

Message ID 20220707153900.3147694-7-emma.finn@intel.com
State Changes Requested
Headers show
Series Actions Infrastructure + Optimizations | expand

Checks

Context Check Description
ovsrobot/apply-robot fail apply and check: fail
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Emma Finn July 7, 2022, 3:38 p.m. UTC
This commit adds the AVX512 implementation of the action functionality.

Usage:
  $ ovs-appctl odp-execute/action-impl-set avx512

Signed-off-by: Emma Finn <emma.finn@intel.com>
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 Documentation/ref/ovs-actions.7.rst | 26 +++++++++++++++++
 Documentation/topics/testing.rst    | 24 ++++++++++------
 NEWS                                |  1 +
 lib/automake.mk                     |  6 +++-
 lib/cpu.c                           |  1 +
 lib/cpu.h                           |  1 +
 lib/odp-execute-avx512.c            | 32 +++++++++++++++++++++
 lib/odp-execute-private.c           | 43 +++++++++++++++++++++++++++++
 lib/odp-execute-private.h           |  8 ++++++
 9 files changed, 133 insertions(+), 9 deletions(-)
 create mode 100644 lib/odp-execute-avx512.c

Comments

0-day Robot July 7, 2022, 4:08 p.m. UTC | #1
Bleep bloop.  Greetings Emma Finn, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


build:
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  g++ -std=gnu++11 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -g -O2 -MT include/openvswitch/cxxtest.lo -MD -MP -MF include/openvswitch/.deps/cxxtest.Tpo -c include/openvswitch/cxxtest.cc -o include/openvswitch/cxxtest.o
/bin/sh ./libtool  --tag=CXX   --mode=link g++ -std=gnu++11  -g -O2     -o include/openvswitch/libcxxtest.la  include/openvswitch/cxxtest.lo  -lpthread -lrt -lm  -lunbound
libtool: link: rm -fr  include/openvswitch/.libs/libcxxtest.a include/openvswitch/.libs/libcxxtest.la
libtool: link: ar cru include/openvswitch/.libs/libcxxtest.a  include/openvswitch/cxxtest.o
libtool: link: ranlib include/openvswitch/.libs/libcxxtest.a
libtool: link: ( cd "include/openvswitch/.libs" && rm -f "libcxxtest.la" && ln -s "../libcxxtest.la" "libcxxtest.la" )
depbase=`echo utilities/ovs-appctl.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -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 -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -MT utilities/ovs-appctl.o -MD -MP -MF $depbase.Tpo -c -o utilities/ovs-appctl.o utilities/ovs-appctl.c &&\
mv -f $depbase.Tpo $depbase.Po
/bin/sh ./libtool  --tag=CC   --mode=link gcc -std=gnu99 -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 -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2     -o utilities/ovs-appctl utilities/ovs-appctl.o lib/libopenvswitch.la -lpthread -lrt -lm  -lunbound
libtool: link: gcc -std=gnu99 -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 -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -o utilities/ovs-appctl utilities/ovs-appctl.o  lib/.libs/libopenvswitch.a -lssl -lcrypto -lcap-ng -lpthread -lrt -lm -lunbound
depbase=`echo utilities/ovs-testcontroller.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -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 -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -MT utilities/ovs-testcontroller.o -MD -MP -MF $depbase.Tpo -c -o utilities/ovs-testcontroller.o utilities/ovs-testcontroller.c &&\
mv -f $depbase.Tpo $depbase.Po
/bin/sh ./libtool  --tag=CC   --mode=link gcc -std=gnu99 -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 -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2     -o utilities/ovs-testcontroller utilities/ovs-testcontroller.o lib/libopenvswitch.la -lssl -lcrypto   -lpthread -lrt -lm  -lunbound
libtool: link: gcc -std=gnu99 -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 -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -o utilities/ovs-testcontroller utilities/ovs-testcontroller.o  lib/.libs/libopenvswitch.a -lcap-ng -lssl -lcrypto -lpthread -lrt -lm -lunbound
lib/.libs/libopenvswitch.a(odp-execute-private.o): In function `action_avx512_probe':
/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace/lib/odp-execute-private.c:60: undefined reference to `action_avx512_init'
collect2: error: ld returned 1 exit status
make[2]: *** [utilities/ovs-testcontroller] Error 1
make[2]: Leaving directory `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/lib/jenkins/jobs/0day_robot_upstream_build_from_pw/workspace'
make: *** [all] Error 2


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Pai G, Sunil July 12, 2022, 11:22 a.m. UTC | #2
Hi Emma, 

Thanks for the patch, couple of comments inline.

<snipped>

> diff --git a/lib/automake.mk b/lib/automake.mk index 5c3b05f6b..e6335ccac
> 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -31,6 +31,9 @@ lib_LTLIBRARIES += lib/libopenvswitchavx512.la
> lib_libopenvswitch_la_LIBADD += lib/libopenvswitchavx512.la
> lib_libopenvswitchavx512_la_CFLAGS = \
>  	-mavx512f \
> +	-mavx512bw \
> +	-mavx512vl \
> +	-mavx512dq \

We don't need these flags here anymore as they are covered below.
Seems like leftovers from rebase, we can remove them.

>  	-mbmi \
>  	-mbmi2 \
>  	-fPIC \
> @@ -44,7 +47,8 @@ lib_libopenvswitchavx512_la_CFLAGS += \
>  	-mavx512vl
>  lib_libopenvswitchavx512_la_SOURCES += \
>  	lib/dpif-netdev-extract-avx512.c \
> -	lib/dpif-netdev-lookup-avx512-gather.c
> +	lib/dpif-netdev-lookup-avx512-gather.c \
> +	lib/odp-execute-avx512.c
>  endif # HAVE_AVX512VL
>  endif # HAVE_AVX512BW

<snipped>

Thanks and regards
Sunil
Emma Finn July 12, 2022, 11:47 a.m. UTC | #3
> -----Original Message-----
> From: Pai G, Sunil <sunil.pai.g@intel.com>
> Sent: Tuesday 12 July 2022 12:22
> To: Finn, Emma <emma.finn@intel.com>; dev@openvswitch.org
> Cc: i.maximets@ovn.org; echaudro@redhat.com; Van Haaren, Harry
> <harry.van.haaren@intel.com>; Amber, Kumar <kumar.amber@intel.com>
> Subject: RE: [ovs-dev] [v8 06/10] odp-execute: Add ISA implementation of
> actions.
> 
> Hi Emma,
> 
> Thanks for the patch, couple of comments inline.
> 
> <snipped>
> 
> > diff --git a/lib/automake.mk b/lib/automake.mk index
> > 5c3b05f6b..e6335ccac
> > 100644
> > --- a/lib/automake.mk
> > +++ b/lib/automake.mk
> > @@ -31,6 +31,9 @@ lib_LTLIBRARIES += lib/libopenvswitchavx512.la
> > lib_libopenvswitch_la_LIBADD += lib/libopenvswitchavx512.la
> > lib_libopenvswitchavx512_la_CFLAGS = \
> >  	-mavx512f \
> > +	-mavx512bw \
> > +	-mavx512vl \
> > +	-mavx512dq \
> 
> We don't need these flags here anymore as they are covered below.
> Seems like leftovers from rebase, we can remove them.

Yes, this change will be removed in next version. 

> 
> >  	-mbmi \
> >  	-mbmi2 \
> >  	-fPIC \
> > @@ -44,7 +47,8 @@ lib_libopenvswitchavx512_la_CFLAGS += \
> >  	-mavx512vl
> >  lib_libopenvswitchavx512_la_SOURCES += \
> >  	lib/dpif-netdev-extract-avx512.c \
> > -	lib/dpif-netdev-lookup-avx512-gather.c
> > +	lib/dpif-netdev-lookup-avx512-gather.c \
> > +	lib/odp-execute-avx512.c
> >  endif # HAVE_AVX512VL
> >  endif # HAVE_AVX512BW
> 
> <snipped>
> 
> Thanks and regards
> Sunil
diff mbox series

Patch

diff --git a/Documentation/ref/ovs-actions.7.rst b/Documentation/ref/ovs-actions.7.rst
index b59b7634f..2410acc4a 100644
--- a/Documentation/ref/ovs-actions.7.rst
+++ b/Documentation/ref/ovs-actions.7.rst
@@ -125,6 +125,32 @@  the one added to the set later replaces the earlier action:
 
 An action set may only contain the actions listed above.
 
+Actions Implementations (Experimental)
+--------------------------------------
+
+Actions are used in OpenFlow flows to describe what to do when the flow
+matches a packet. Just like with the datapath interface, SIMD instructions
+with the userspace datapath can be applied to the action implementation to
+improve performance.
+
+OVS provides multiple implementations of the actions.
+Available implementations can be listed with the following command::
+
+    $ ovs-appctl odp-execute/action-impl-show
+        Available Actions implementations:
+            scalar (available: Yes, active: Yes)
+            autovalidator (available: Yes, active: No)
+            avx512 (available: Yes, active: No)
+
+By default, ``scalar`` is used.  Implementations can be selected by
+name::
+
+    $ ovs-appctl odp-execute/action-impl-set avx512
+    Action implementation set to avx512.
+
+    $ ovs-appctl odp-execute/action-impl-set scalar
+    Action implementation set to scalar.
+
 Error Handling
 --------------
 
diff --git a/Documentation/topics/testing.rst b/Documentation/topics/testing.rst
index c15d5b38f..a6c747b18 100644
--- a/Documentation/topics/testing.rst
+++ b/Documentation/topics/testing.rst
@@ -361,12 +361,12 @@  testsuite.
 Userspace datapath: Testing and Validation of CPU-specific Optimizations
 ''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
-As multiple versions of the datapath classifier and packet parsing functions
-can co-exist, each with different CPU ISA optimizations, it is important to
-validate that they all give the exact same results.  To easily test all the
-implementations, an ``autovalidator`` implementation of them exists.  This
-implementation runs all other available implementations, and verifies that the
-results are identical.
+As multiple versions of the datapath classifier, packet parsing functions and
+actions can co-exist, each with different CPU ISA optimizations, it is
+important to validate that they all give the exact same results.  To easily
+test all the implementations, an ``autovalidator`` implementation of them
+exists. This implementation runs all other available implementations, and
+verifies that the results are identical.
 
 Running the OVS unit tests with the autovalidator enabled ensures all
 implementations provide the same results.  Note that the performance of the
@@ -382,18 +382,26 @@  To set the autovalidator for the packet parser, use this command::
 
     $ ovs-appctl dpif-netdev/miniflow-parser-set autovalidator
 
+To set the autovalidator for actions, use this command::
+
+    $ ovs-appctl odp-execute/action-impl-set autovalidator
+
 To run the OVS unit test suite with the autovalidator as the default
 implementation, it is required to recompile OVS.  During the recompilation,
 the default priority of the `autovalidator` implementation is set to the
-maximum priority, ensuring every test will be run with every implementation::
+maximum priority, ensuring every test will be run with every implementation.
+Priority is only related to mfex autovalidator and not the actions
+autovalidator.::
 
-    $ ./configure --enable-autovalidator --enable-mfex-default-autovalidator
+    $ ./configure --enable-autovalidator --enable-mfex-default-autovalidator \
+        --enable-actions-default-autovalidator
 
 The following line should be seen in the configuration log when the above
 options are used::
 
     checking whether DPCLS Autovalidator is default implementation... yes
     checking whether MFEX Autovalidator is default implementation... yes
+    checking whether actions Autovalidator is default implementation... yes
 
 Compile OVS in debug mode to have `ovs_assert` statements error out if
 there is a mis-match in the datapath classifier lookup or packet parser
diff --git a/NEWS b/NEWS
index 607514874..751951ac9 100644
--- a/NEWS
+++ b/NEWS
@@ -49,6 +49,7 @@  Post-v2.17.0
        implementations available at run time.
      * Add build time configure command to enable auto-validator as default
        actions implementation at build time.
+     * Add AVX512 implementation of actions.
 
 
 v2.17.0 - 17 Feb 2022
diff --git a/lib/automake.mk b/lib/automake.mk
index 5c3b05f6b..e6335ccac 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -31,6 +31,9 @@  lib_LTLIBRARIES += lib/libopenvswitchavx512.la
 lib_libopenvswitch_la_LIBADD += lib/libopenvswitchavx512.la
 lib_libopenvswitchavx512_la_CFLAGS = \
 	-mavx512f \
+	-mavx512bw \
+	-mavx512vl \
+	-mavx512dq \
 	-mbmi \
 	-mbmi2 \
 	-fPIC \
@@ -44,7 +47,8 @@  lib_libopenvswitchavx512_la_CFLAGS += \
 	-mavx512vl
 lib_libopenvswitchavx512_la_SOURCES += \
 	lib/dpif-netdev-extract-avx512.c \
-	lib/dpif-netdev-lookup-avx512-gather.c
+	lib/dpif-netdev-lookup-avx512-gather.c \
+	lib/odp-execute-avx512.c
 endif # HAVE_AVX512VL
 endif # HAVE_AVX512BW
 lib_libopenvswitchavx512_la_LDFLAGS = \
diff --git a/lib/cpu.c b/lib/cpu.c
index 2df003c51..0292f715e 100644
--- a/lib/cpu.c
+++ b/lib/cpu.c
@@ -53,6 +53,7 @@  X86_ISA(X86_EXT_FEATURES_LEAF, EBX, 16, OVS_CPU_ISA_X86_AVX512F)
 X86_ISA(X86_EXT_FEATURES_LEAF, EBX, 30, OVS_CPU_ISA_X86_AVX512BW)
 X86_ISA(X86_EXT_FEATURES_LEAF, ECX,  1, OVS_CPU_ISA_X86_AVX512VBMI)
 X86_ISA(X86_EXT_FEATURES_LEAF, ECX, 14, OVS_CPU_ISA_X86_VPOPCNTDQ)
+X86_ISA(X86_EXT_FEATURES_LEAF, EBX, 31, OVS_CPU_ISA_X86_AVX512VL)
 #endif
 
 bool
diff --git a/lib/cpu.h b/lib/cpu.h
index 92897bb71..3215229bc 100644
--- a/lib/cpu.h
+++ b/lib/cpu.h
@@ -25,6 +25,7 @@  enum ovs_cpu_isa {
     OVS_CPU_ISA_X86_AVX512F,
     OVS_CPU_ISA_X86_AVX512BW,
     OVS_CPU_ISA_X86_AVX512VBMI,
+    OVS_CPU_ISA_X86_AVX512VL,
     OVS_CPU_ISA_X86_VPOPCNTDQ,
     OVS_CPU_ISA_X86_LAST = OVS_CPU_ISA_X86_VPOPCNTDQ,
 };
diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
new file mode 100644
index 000000000..33c9078cf
--- /dev/null
+++ b/lib/odp-execute-avx512.c
@@ -0,0 +1,32 @@ 
+/*
+ * Copyright (c) 2022 Intel.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <config.h>
+#include <errno.h>
+
+#include "dp-packet.h"
+#include "immintrin.h"
+#include "odp-execute-private.h"
+#include "odp-netlink.h"
+#include "openvswitch/vlog.h"
+
+int
+action_avx512_init(struct odp_execute_action_impl *self OVS_UNUSED)
+{
+    /* Set function pointers for actions that can be applied directly, these
+     * are identified by OVS_ACTION_ATTR_*. */
+    return 0;
+}
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index 442837fa5..d99a94a93 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -19,6 +19,7 @@ 
 #include <stdio.h>
 #include <string.h>
 
+#include "cpu.h"
 #include "dpdk.h"
 #include "dp-packet.h"
 #include "odp-execute-private.h"
@@ -29,6 +30,40 @@ 
 VLOG_DEFINE_THIS_MODULE(odp_execute_impl);
 static int active_action_impl_index;
 
+#ifdef ACTION_IMPL_AVX512_CHECK
+/* Probe functions to check ISA requirements. */
+static bool
+action_avx512_isa_probe(void)
+{
+    static enum ovs_cpu_isa isa_required[] = {
+        OVS_CPU_ISA_X86_AVX512F,
+        OVS_CPU_ISA_X86_AVX512BW,
+        OVS_CPU_ISA_X86_BMI2,
+        OVS_CPU_ISA_X86_AVX512VL,
+    };
+
+    for (int i = 0; i < ARRAY_SIZE(isa_required); i++) {
+        if (!cpu_has_isa(isa_required[i])) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
+static int
+action_avx512_probe(struct odp_execute_action_impl *self)
+{
+    if (!action_avx512_isa_probe()) {
+        return -ENOTSUP;
+    } else {
+        action_avx512_init(self);
+    }
+
+    return 0;
+}
+#endif
+
 static struct odp_execute_action_impl action_impls[] = {
     [ACTION_IMPL_AUTOVALIDATOR] = {
         .available = false,
@@ -41,6 +76,14 @@  static struct odp_execute_action_impl action_impls[] = {
         .name = "scalar",
         .init_func = odp_action_scalar_init,
     },
+
+#ifdef ACTION_IMPL_AVX512_CHECK
+    [ACTION_IMPL_AVX512] = {
+        .available = false,
+        .name = "avx512",
+        .init_func = action_avx512_probe,
+    },
+#endif
 };
 
 static void
diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
index d6eebbf37..3ece71e7b 100644
--- a/lib/odp-execute-private.h
+++ b/lib/odp-execute-private.h
@@ -22,6 +22,9 @@ 
 #include "odp-netlink.h"
 #include "ovs-atomic.h"
 
+#define ACTION_IMPL_AVX512_CHECK (__x86_64__ && HAVE_AVX512F \
+    && HAVE_LD_AVX512_GOOD && __SSE4_2__)
+
 /* Forward declaration for typedef. */
 struct odp_execute_action_impl;
 
@@ -59,6 +62,9 @@  enum odp_execute_action_impl_idx {
      * Do not change the autovalidator position in this list without updating
      * the define below.
      */
+#ifdef ACTION_IMPL_AVX512_CHECK
+    ACTION_IMPL_AVX512,
+#endif
 
     ACTION_IMPL_MAX,
 };
@@ -84,6 +90,8 @@  struct odp_execute_action_impl * odp_execute_action_set(const char *name);
 
 int action_autoval_init(struct odp_execute_action_impl *self);
 
+int action_avx512_init(struct odp_execute_action_impl *self OVS_UNUSED);
+
 void odp_execute_action_get_info(struct ds *name);
 
 #endif /* ODP_EXTRACT_PRIVATE */