[ovs-dev,v7,07/11] odp-execute: Add ISA implementation of actions.

This commit adds the AVX512 implementation of the action functionality.

Usage:
  $ ovs-appctl dpif-netdev/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                                |  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

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 <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                                |  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",

On 23 Jun 2022, at 17:38, Eelco Chaudron wrote:

> 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 <emma.finn@intel.com>
>> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Please also take a look at https://patchwork.ozlabs.org/project/openvswitch/patch/20220624072959.240183-1-david.marchand@redhat.com/ as the same fix is needed here.