On 10 May 2022, at 16:21, 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/topics/dpdk/bridge.rst | 25 +++++++++++
> Documentation/topics/testing.rst | 20 ++++++---
> NEWS | 1 +
> lib/automake.mk | 4 +-
> lib/cpu.c | 1 +
> lib/cpu.h | 1 +
> lib/odp-execute-avx512.c | 67 ++++++++++++++++++++++++++++
> lib/odp-execute-private.c | 9 ++++
> lib/odp-execute-private.h | 9 ++++
> 9 files changed, 129 insertions(+), 8 deletions(-)
> create mode 100644 lib/odp-execute-avx512.c
>
> diff --git a/Documentation/topics/dpdk/bridge.rst
> b/Documentation/topics/dpdk/bridge.rst
> index ceee91015..67089e08f 100644
> --- a/Documentation/topics/dpdk/bridge.rst
> +++ b/Documentation/topics/dpdk/bridge.rst
This is not DPDK specific, so it should be in some other documentation.
> @@ -321,3 +321,28 @@ following command::
> ``scalar`` can be selected on core ``3`` by the following command::
>
> $ ovs-appctl dpif-netdev/miniflow-parser-set -pmd 3 scalar
> +
> +Actions Performance
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-get
action-impl-show, and update output below based on review changes.
> + Available Actions implementations:
> + scalar (available: True, active: True)
> + autovalidator (available: True, active: False)
> + avx512 (available: True, active: False)
> +
> +By default, ``scalar`` is used. Implementations can be selected by
> +name::
> +
> + $ ovs-appctl dpif-netdev/action-impl-set avx512
> + action implementation set to avx512.
Update text here based on review changes (same below).
> +
> + $ ovs-appctl dpif-netdev/action-impl-set scalar
> + action implementation set to scalar.
> diff --git a/Documentation/topics/testing.rst
> b/Documentation/topics/testing.rst
> index c15d5b38f..10d0ecc48 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,24 @@ 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
Do we need some re-write here that the prioriry only effects mfex?
> maximum priority, ensuring every test will be run with every implementation::
>
> - $ ./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 73796e4d5..35d6b0f4a 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -64,6 +64,7 @@ v2.17.0 - 17 Feb 2022
> 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.
> - Python:
> * For SSL support, the use of the pyOpenSSL library has been replaced
> with the native 'ssl' module.
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 625c0d9c9..2973ec12d 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -32,6 +32,7 @@ lib_libopenvswitch_la_LIBADD += lib/libopenvswitchavx512.la
> lib_libopenvswitchavx512_la_CFLAGS = \
> -mavx512f \
> -mavx512bw \
> + -mavx512vl \
> -mavx512dq \
> -mbmi \
> -mbmi2 \
> @@ -42,7 +43,8 @@ lib_libopenvswitchavx512_la_SOURCES = \
> lib/cpu.h \
> lib/dpif-netdev-lookup-avx512-gather.c \
> lib/dpif-netdev-extract-avx512.c \
> - lib/dpif-netdev-avx512.c
> + lib/dpif-netdev-avx512.c \
> + lib/odp-execute-avx512.c
> lib_libopenvswitchavx512_la_LDFLAGS = \
> -static
> endif
> 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..84f68d378
> --- /dev/null
> +++ b/lib/odp-execute-avx512.c
> @@ -0,0 +1,67 @@
> +/*
> + * 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 int32_t
int
> +avx512_isa_probe(uint32_t needs_vbmi)
This should be a bool.
> +{
> + 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
> + };
> +
> + int32_t ret = 0;
int
> + for (uint32_t i = 0; i < ARRAY_SIZE(isa_required); i++) {
int i = 0;
> + if (!cpu_has_isa(isa_required[i])) {
> + ret = -ENOTSUP;
> + }
> + }
> +
> + if (needs_vbmi) {
This is not used anywhere in the patchset, so I think it should be removed for
now.
> + if (!cpu_has_isa(OVS_CPU_ISA_X86_AVX512VBMI)) {
> + ret = -ENOTSUP;
> + }
> + }
> +
> + return ret;
> +}
> +
> +int32_t
> +action_avx512_probe(void)
> +{
> + const uint32_t needs_vbmi = 0;
Why this odd definition? I would just use false in the call below.
> + return avx512_isa_probe(needs_vbmi);
The avx512_isa_probe() function name is rather generic, it does not indicate
its action implementation related.
Maybe the code of this function can just be moved here?
> +}
> +
> +
> +int32_t
int
> +action_avx512_init(void)
> +{
> + avx512_isa_probe(0);
Why do we call this probe function at all here? Is there a specific need? Also
(0) should be (false).
> + return 0;
> +}
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> index 8f5f8723f..2bfa84152 100644
> --- a/lib/odp-execute-private.c
> +++ b/lib/odp-execute-private.c
> @@ -46,6 +46,15 @@ static struct odp_execute_action_impl action_impls[] = {
> .probe = NULL,
> .init_func = odp_action_scalar_init,
> },
> +
> + #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
How about changing this instance to #ifdef ACTION_IMPL_AVX512? This way we only
have one place where we have these compiler/arch checks.
> + [ACTION_IMPL_AVX512] = {
> + .available = 1,
This should be “= false”, as AVX is not available in most cases.
> + .name = "avx512",
> + .probe = action_avx512_probe,
> + .init_func = NULL,
Why is this NULL? You defined an init function (with a missing argument) above
so you should use it.
> + },
> + #endif
> };
>
> static void
> diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
> index fed20930d..13fc74e52 100644
> --- a/lib/odp-execute-private.h
> +++ b/lib/odp-execute-private.h
> @@ -71,6 +71,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,
> };
> @@ -96,4 +99,10 @@ int32_t odp_execute_action_set(const char *name,
> */
> int32_t odp_action_scalar_init(struct odp_execute_action_impl *self);
>
> +/* Init function for the optimized with AVX512 actions. */
> +int32_t action_avx512_init(void);
int
> +
> +/* Probe function to check ISA requirements. */
> +int32_t action_avx512_probe(void);
int
> +
> #endif /* ODP_EXTRACT_PRIVATE */
> --
> 2.25.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
