Message ID | 20220707153900.3147694-7-emma.finn@intel.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Actions Infrastructure + Optimizations | expand |
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 |
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
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
> -----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 --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 */