diff mbox series

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

Message ID 20220712174456.2682549-7-harry.van.haaren@intel.com
State Changes Requested
Headers show
Series Actions Infrastructure + Optimizations | expand

Checks

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

Commit Message

Van Haaren, Harry July 12, 2022, 5:44 p.m. UTC
From: Emma Finn <emma.finn@intel.com>

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>

---

v9: rebase conflict on NEWS
---
 Documentation/ref/ovs-actions.7.rst | 26 ++++++++++++++++++++++++++
 Documentation/topics/testing.rst    | 24 ++++++++++++++++--------
 NEWS                                |  1 +
 lib/cpu.c                           |  1 +
 lib/cpu.h                           |  1 +
 lib/odp-execute-private.c           |  8 ++++++++
 lib/odp-execute-private.h           |  6 ++++++
 7 files changed, 59 insertions(+), 8 deletions(-)

Comments

Ilya Maximets July 12, 2022, 11:24 p.m. UTC | #1
On 7/12/22 19:44, Harry van Haaren wrote:
> From: Emma Finn <emma.finn@intel.com>
> 
> 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>
> 
> ---
> 
> v9: rebase conflict on NEWS
> ---
>  Documentation/ref/ovs-actions.7.rst | 26 ++++++++++++++++++++++++++
>  Documentation/topics/testing.rst    | 24 ++++++++++++++++--------
>  NEWS                                |  1 +
>  lib/cpu.c                           |  1 +
>  lib/cpu.h                           |  1 +
>  lib/odp-execute-private.c           |  8 ++++++++
>  lib/odp-execute-private.h           |  6 ++++++
>  7 files changed, 59 insertions(+), 8 deletions(-)
> 
> 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.

odp-execute doesn't execute OpenFlow actions, it executes datapath flow
actions.  These are very different things.  So, the paragraph above is
not correct.

And the ovs-actions man page is about OpenFlow actions supported by OVS.
It's about format of OpenFlow actions and the processing of the OpenFlow
pipeline.  It's not about datapath actions nor any other aspect of actions.

This piece of documentation should be fixed and it should not be here.

(Accidentally noticed that while trying to report build failure)

Better move that back to Documentation/topics/dpdk/bridge.rst.
It's not DPDK-specific, but there is actually nuothing DPDK-specific
in the bridge.rst.  We need to rename that document and move to some
other palace at some point.

Best regards, Ilya Maximets.
Van Haaren, Harry July 13, 2022, 11:24 a.m. UTC | #2
> -----Original Message-----
> From: Ilya Maximets <i.maximets@ovn.org>
> Sent: Wednesday, July 13, 2022 12:25 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@openvswitch.org
> Cc: i.maximets@ovn.org; echaudro@redhat.com; Amber, Kumar
> <kumar.amber@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Finn, Emma
> <emma.finn@intel.com>; Stokes, Ian <ian.stokes@intel.com>
> Subject: Re: [PATCH v9 06/10] odp-execute: Add ISA implementation of actions.
> 
> On 7/12/22 19:44, Harry van Haaren wrote:
> > From: Emma Finn <emma.finn@intel.com>
> >
> > 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>
> >
> > ---
> >
> > v9: rebase conflict on NEWS
> > ---
> >  Documentation/ref/ovs-actions.7.rst | 26 ++++++++++++++++++++++++++
> >  Documentation/topics/testing.rst    | 24 ++++++++++++++++--------
> >  NEWS                                |  1 +
> >  lib/cpu.c                           |  1 +
> >  lib/cpu.h                           |  1 +
> >  lib/odp-execute-private.c           |  8 ++++++++
> >  lib/odp-execute-private.h           |  6 ++++++
> >  7 files changed, 59 insertions(+), 8 deletions(-)
> >
> > 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.
> 
> odp-execute doesn't execute OpenFlow actions, it executes datapath flow
> actions.  These are very different things.  So, the paragraph above is
> not correct.

Reworded, simplifying language and focus on datapath.

> And the ovs-actions man page is about OpenFlow actions supported by OVS.
> It's about format of OpenFlow actions and the processing of the OpenFlow
> pipeline.  It's not about datapath actions nor any other aspect of actions.
> 
> This piece of documentation should be fixed and it should not be here.

Moved back to bridge.rst as suggested below.

> (Accidentally noticed that while trying to report build failure)
> 
> Better move that back to Documentation/topics/dpdk/bridge.rst.
> It's not DPDK-specific, but there is actually nuothing DPDK-specific
> in the bridge.rst.  We need to rename that document and move to some
> other palace at some point.

Above feedback makes sense, will be included in v10, and sent shortly.

Leaving moving/renaming topics/dpdk/bridge.rst out, as not related to Actions patchset.
Noted for future improvement.

> Best regards, Ilya Maximets.

Regards, -Harry
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 2359b6bcf..fa2f7d535 100644
--- a/NEWS
+++ b/NEWS
@@ -55,6 +55,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.
    - Linux datapath:
      * Add offloading meter tc police.
 
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-private.c b/lib/odp-execute-private.c
index 38be22ec9..f7fb60467 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -41,6 +41,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 = NULL,
+    },
+#endif
 };
 
 static void
diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
index d6eebbf37..1c636faeb 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,
 };