On 14 Jun 2022, at 13:57, Emma Finn wrote:
> This commit adds the AVX512 implementation of the action functionality.
>
> Usage:
> $ ovs-appctl dpif-netdev/action-impl-set avx512
>
> Signed-off-by: Emma Finn <[email protected]>
> Acked-by: Harry van Haaren <[email protected]>
> ---
> Documentation/ref/ovs-actions.7.rst | 26 ++++++++++++++
> Documentation/topics/testing.rst | 24 ++++++++-----
> NEWS | 2 ++
> lib/automake.mk | 6 +++-
> lib/cpu.c | 1 +
> lib/cpu.h | 1 +
> lib/odp-execute-avx512.c | 56 +++++++++++++++++++++++++++++
> lib/odp-execute-private.c | 8 +++++
> lib/odp-execute-private.h | 6 +++-
> 9 files changed, 120 insertions(+), 10 deletions(-)
> create mode 100644 lib/odp-execute-avx512.c
>
> diff --git a/Documentation/ref/ovs-actions.7.rst
> b/Documentation/ref/ovs-actions.7.rst
> index b59b7634f..a363d1571 100644
> --- a/Documentation/ref/ovs-actions.7.rst
> +++ b/Documentation/ref/ovs-actions.7.rst
> @@ -2354,3 +2354,29 @@ never leads to recursion. The ``resubmit`` extension
> action is more flexible.
> **Conformance**
> OpenFlow 1.1 introduced ``goto_table``. Open vSwitch 2.1 added support for
> ``goto_table``.
> +
> +Actions Implementations
> +-----------------------
> +
> +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 dpif-netdev/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 dpif-netdev/action-impl-set avx512
> + Action implementation set to avx512.
> +
> + $ ovs-appctl dpif-netdev/action-impl-set scalar
> + Action implementation set to scalar.
The location in the document is a bit odd, as it now looks part of the
"Instructions" section.
What about moving it here:
diff --git a/Documentation/ref/ovs-actions.7.rst
b/Documentation/ref/ovs-actions.7.rst
index a363d1571..c07438a2f 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
+-----------------------
....
> diff --git a/Documentation/topics/testing.rst
> b/Documentation/topics/testing.rst
> index c15d5b38f..d279131b8 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 dpif-netdev/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 7ae4d87bb..927b7eed8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -39,6 +39,8 @@ 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.
> +
>
>
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 1a49dd30b..ad3ea3fb1 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 \
Indentation if off.
> -mbmi \
> -mbmi2 \
> -fPIC \
> @@ -44,7 +47,8 @@ lib_libopenvswitchavx512_la_CFLAGS += \
> -mavx512bw
> 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_AVX512BW
> lib_libopenvswitchavx512_la_LDFLAGS = \
> -static
> 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..1fb334689
> --- /dev/null
> +++ b/lib/odp-execute-avx512.c
> @@ -0,0 +1,56 @@
> +/*
> + * 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 "cpu.h"
> +#include "dp-packet.h"
> +#include "immintrin.h"
> +#include "odp-execute-private.h"
> +#include "odp-netlink.h"
> +#include "openvswitch/vlog.h"
> +
> +/* Probe functions to check ISA requirements. */
> +static bool
> +avx512_isa_probe(void)
Please rename this function to an action specific one, to avoid name conflicts.
For example, action_avx512_isa_probe()
> +{
> + 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,
> + };
> +
> + bool ret = true;
> + for (int i = 0; i < ARRAY_SIZE(isa_required); i++) {
> + if (!cpu_has_isa(isa_required[i])) {
> + ret = -ENOTSUP;
The return is a bool, so how can it be -ENOTSUP?
Guess we should just return false here.
> + }
> + }
> +
> + return ret;
and return true here.
> +}
> +
> +int
> +action_avx512_init(struct odp_execute_action_impl *self)
> +{
> + if (!avx512_isa_probe()) {
> + return -ENOTSUP;
> + }
> +
> + return 0;
> +}
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> index f8d0896b5..de2e4dfc4 100644
> --- a/lib/odp-execute-private.c
> +++ b/lib/odp-execute-private.c
> @@ -42,6 +42,14 @@ static struct odp_execute_action_impl action_impls[] = {
> .name = "scalar",
> .init_func = odp_action_scalar_init,
> },
> +
> + #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
>From the v6 discussion:
EC> How about changing this instance to #ifdef ACTION_IMPL_AVX512? This way we
only have one place where we have these compiler/arch checks.
EF> V7 will not include a fix here, but we will investigate and report back to
OVS ML with results of investigation.
Not sure why you need further investigation? If ACTION_IMPL_AVX512 was not
defined the above flags where false already in odp-execute-private.h.
I think the change should simply be this:
- #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
+ #ifdef ACTION_IMPL_AVX512
Or am I missing something? I see one version on gcc complain about this, is
this what you are figuring out?
gcc (GCC) 11.3.1 20220421 works fine, gcc (GCC) 11.2.1 20220127 seems to report
an issues:
lib/odp-execute-private.c:86:9: warning: iteration 2 invokes undefined
behavior [-Waggressive-loop-optimizations]
I do not see this problem with clang. Also the github actions script compile
just fine. You might just be as unlucky as I was, and you have a broken compile
version?
> + [ACTION_IMPL_AVX512] = {
> + .available = false,
> + .name = "avx512",
> + .init_func = action_avx512_init,
> + },
> + #endif
> };
>
> static void
> diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
> index 5322eb8df..e4724b8b2 100644
> --- a/lib/odp-execute-private.h
> +++ b/lib/odp-execute-private.h
> @@ -59,6 +59,9 @@ enum odp_execute_action_impl_idx {
> * Do not change the autovalidator position in this list without updating
> * the define below.
> */
> + #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
> + ACTION_IMPL_AVX512,
> + #endif
>
> ACTION_IMPL_MAX,
> };
> @@ -82,10 +85,11 @@ int odp_action_scalar_init(struct odp_execute_action_impl
> *self);
>
> int action_autoval_init(struct odp_execute_action_impl *self);
>
> +int action_avx512_init(struct odp_execute_action_impl *self);
Guess you did not compile your patches individually? They should pass
compilation and existing tests, so gitdissection will always work.
+action_avx512_init(struct odp_execute_action_impl *self OVS_UNUSED)
> +
> int odp_execute_action_set(const char *name,
> struct odp_execute_action_impl *active);
>
> void odp_execute_action_get_info(struct ds *name);
>
> -
> #endif /* ODP_EXTRACT_PRIVATE */
> --
> 2.32.0
Here is my full diff:
diff --git a/Documentation/ref/ovs-actions.7.rst
b/Documentation/ref/ovs-actions.7.rst
index a363d1571..c07438a2f 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
+-----------------------
+
+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 dpif-netdev/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 dpif-netdev/action-impl-set avx512
+ Action implementation set to avx512.
+
+ $ ovs-appctl dpif-netdev/action-impl-set scalar
+ Action implementation set to scalar.
+
Error Handling
--------------
@@ -2354,29 +2380,3 @@ never leads to recursion. The ``resubmit`` extension
action is more flexible.
**Conformance**
OpenFlow 1.1 introduced ``goto_table``. Open vSwitch 2.1 added support for
``goto_table``.
-
-Actions Implementations
------------------------
-
-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 dpif-netdev/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 dpif-netdev/action-impl-set avx512
- Action implementation set to avx512.
-
- $ ovs-appctl dpif-netdev/action-impl-set scalar
- Action implementation set to scalar.
diff --git a/lib/automake.mk b/lib/automake.mk
index ad3ea3fb1..d6545c839 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -32,8 +32,8 @@ lib_libopenvswitch_la_LIBADD += lib/libopenvswitchavx512.la
lib_libopenvswitchavx512_la_CFLAGS = \
-mavx512f \
-mavx512bw \
- -mavx512vl \
- -mavx512dq \
+ -mavx512vl \
+ -mavx512dq \
-mbmi \
-mbmi2 \
-fPIC \
diff --git a/lib/odp-execute-avx512.c b/lib/odp-execute-avx512.c
index 1fb334689..b7457bf14 100644
--- a/lib/odp-execute-avx512.c
+++ b/lib/odp-execute-avx512.c
@@ -26,7 +26,7 @@
/* Probe functions to check ISA requirements. */
static bool
-avx512_isa_probe(void)
+action_avx512_isa_probe(void)
{
static enum ovs_cpu_isa isa_required[] = {
OVS_CPU_ISA_X86_AVX512F,
@@ -35,20 +35,19 @@ avx512_isa_probe(void)
OVS_CPU_ISA_X86_AVX512VL,
};
- bool ret = true;
for (int i = 0; i < ARRAY_SIZE(isa_required); i++) {
if (!cpu_has_isa(isa_required[i])) {
- ret = -ENOTSUP;
+ return false;
}
}
- return ret;
+ return true;
}
int
-action_avx512_init(struct odp_execute_action_impl *self)
+action_avx512_init(struct odp_execute_action_impl *self OVS_UNUSED)
{
- if (!avx512_isa_probe()) {
+ if (!action_avx512_isa_probe()) {
return -ENOTSUP;
}
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index 4152d6d8e..abf0e04b8 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -42,7 +42,7 @@ static struct odp_execute_action_impl action_impls[] = {
.init_func = odp_action_scalar_init,
},
- #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
+ #ifdef ACTION_IMPL_AVX512
[ACTION_IMPL_AVX512] = {
.available = false,
.name = "avx512",
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
