diff mbox series

[ovs-dev,v7,05/11] odp-execute: Add command to switch action implementation.

Message ID 20220614115743.1143341-6-emma.finn@intel.com
State Changes Requested
Headers show
Series [ovs-dev,v7,01/11] ofproto-dpif: Fix incorrect checksums in input packets | 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

Emma Finn June 14, 2022, 11:57 a.m. UTC 
This commit adds a new command to allow the user to switch
the active action implementation at runtime. A probe function
is executed before switching the implementation, to ensure
the CPU is capable of running the ISA required.

Usage:
  $ ovs-appctl dpif-netdev/action-impl-set scalar

This commit also adds a new command to retrieve the list of available
action implementations. This can be used by to check what implementations
of actions are available and what implementation is active during runtime.

Usage:
   $ ovs-appctl dpif-netdev/action-impl-show

Added separate test-case for ovs-actions show/set commands:
1023: PMD - ovs-actions configuration

Signed-off-by: Emma Finn <emma.finn@intel.com>
Co-authored-by: Kumar Amber <kumar.amber@intel.com>
Signed-off-by: Kumar Amber <kumar.amber@intel.com>
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 NEWS                        |  3 +++
 lib/dpif-netdev-unixctl.man |  8 ++++++++
 lib/dpif-netdev.c           | 38 +++++++++++++++++++++++++++++++++++++
 lib/odp-execute-private.c   | 12 ++++++++++++
 lib/odp-execute-private.h   |  3 +++
 lib/odp-execute.h           |  2 ++
 tests/pmd.at                | 30 +++++++++++++++++++++++++++++
 7 files changed, 96 insertions(+)

Comments

Eelco Chaudron June 23, 2022, 3:38 p.m. UTC | #1 
On 14 Jun 2022, at 13:57, Emma Finn wrote:

> This commit adds a new command to allow the user to switch
> the active action implementation at runtime. A probe function
> is executed before switching the implementation, to ensure
> the CPU is capable of running the ISA required.
>
> Usage:
>   $ ovs-appctl dpif-netdev/action-impl-set scalar
>
> This commit also adds a new command to retrieve the list of available
> action implementations. This can be used by to check what implementations
> of actions are available and what implementation is active during runtime.
>
> Usage:
>    $ ovs-appctl dpif-netdev/action-impl-show
>
> Added separate test-case for ovs-actions show/set commands:
> 1023: PMD - ovs-actions configuration
>
> Signed-off-by: Emma Finn <emma.finn@intel.com>
> Co-authored-by: Kumar Amber <kumar.amber@intel.com>
> Signed-off-by: Kumar Amber <kumar.amber@intel.com>
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  NEWS                        |  3 +++
>  lib/dpif-netdev-unixctl.man |  8 ++++++++
>  lib/dpif-netdev.c           | 38 +++++++++++++++++++++++++++++++++++++
>  lib/odp-execute-private.c   | 12 ++++++++++++
>  lib/odp-execute-private.h   |  3 +++
>  lib/odp-execute.h           |  2 ++
>  tests/pmd.at                | 30 +++++++++++++++++++++++++++++
>  7 files changed, 96 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index 3a25f3035..90ceabd63 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -35,6 +35,9 @@ Post-v2.17.0
>     - Userspace datapath:
>       * Add actions auto-validator function to compare different actions
>         implementations against default implementation.
> +     * Add command line option to switch between different actions
> +       implementations available at run time.
> +
>
>
>  v2.17.0 - 17 Feb 2022
> diff --git a/lib/dpif-netdev-unixctl.man b/lib/dpif-netdev-unixctl.man
> index 8cd847416..81ef7d856 100644
> --- a/lib/dpif-netdev-unixctl.man
> +++ b/lib/dpif-netdev-unixctl.man
> @@ -262,3 +262,11 @@ PMDs in the case where no value is specified.  By default "scalar" is used.
>  \fIstudy_cnt\fR defaults to 128 and indicates the number of packets that the
>  "study" miniflow implementation must parse before choosing an optimal
>  implementation.
> +
> +.IP "\fBdpif-netdev/action-impl-show\fR
> +Lists the actions implementations that are available and highlights the
> +currently enabled one.
> +.
> +.IP "\fBdpif-netdev/action-impl-set\fR \fIaction_impl\fR"
> +Sets the action implementation to any available implementation. By default
> +"scalar" is used.
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 47dd7a1a6..5a35c7ce5 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -63,6 +63,7 @@
>  #include "netdev-vport.h"
>  #include "netlink.h"
>  #include "odp-execute.h"
> +#include "odp-execute-private.h"
>  #include "odp-util.h"
>  #include "openvswitch/dynamic-string.h"
>  #include "openvswitch/list.h"
> @@ -1387,6 +1388,37 @@ error:
>      ds_destroy(&reply);
>  }
>
> +static void
> +action_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +                const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +{
> +    struct ds reply = DS_EMPTY_INITIALIZER;
> +
> +    int err = odp_actions_impl_set(argv[1]);
> +    if (err) {
> +        ds_put_format(&reply,
> +                      "Error: unknown action implementation, %s, specified!\n",

Remove "\n" here.

> +                      argv[1]);
> +        unixctl_command_reply_error(conn, ds_cstr(&reply));
> +    } else {
> +        ds_put_format(&reply, "Action implementation set to %s.\n", argv[1]);

Remove "\n" here.

> +        unixctl_command_reply(conn, ds_cstr(&reply));
> +    }
> +
> +    ds_destroy(&reply);
> +}
> +
> +static void
> +action_impl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +                const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +{
> +    struct ds reply = DS_EMPTY_INITIALIZER;
> +
> +    odp_execute_action_get_info(&reply);
> +    unixctl_command_reply(conn, ds_cstr(&reply));
> +    ds_destroy(&reply);
> +}
> +
>  static void
>  dpif_netdev_pmd_rebalance(struct unixctl_conn *conn, int argc,
>                            const char *argv[], void *aux OVS_UNUSED)
> @@ -1624,6 +1656,12 @@ dpif_netdev_init(void)
>      unixctl_command_register("dpif-netdev/miniflow-parser-get", "",
>                               0, 0, dpif_miniflow_extract_impl_get,
>                               NULL);
> +    unixctl_command_register("dpif-netdev/action-impl-set", "name",
> +                             1, 1, action_impl_set,
> +                             NULL);
> +    unixctl_command_register("dpif-netdev/action-impl-show", "",
> +                             0, 0, action_impl_show,
> +                             NULL);
>      return 0;
>  }
>
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> index 267f32c3e..f8d0896b5 100644
> --- a/lib/odp-execute-private.c
> +++ b/lib/odp-execute-private.c
> @@ -68,6 +68,18 @@ odp_execute_action_set(const char *name,
>      return -EINVAL;
>  }
>
> +void
> +odp_execute_action_get_info(struct ds *string)
> +{
> +    ds_put_cstr(string, "Available Actions implementations:\n");
> +    for (int i = 0; i < ACTION_IMPL_MAX; i++) {
> +        ds_put_format(string, "  %s (available: %s, active: %s)\n",
> +                      action_impls[i].name,
> +                      action_impls[i].available ? "Yes" : "No",
> +                      i == active_action_impl_index ? "Yes" : "No");
> +    }
> +}
> +
>  void
>  odp_execute_action_init(void)
>  {
> diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
> index d3dc669d1..5322eb8df 100644
> --- a/lib/odp-execute-private.h
> +++ b/lib/odp-execute-private.h
> @@ -85,4 +85,7 @@ int action_autoval_init(struct odp_execute_action_impl *self);
>  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 */
> diff --git a/lib/odp-execute.h b/lib/odp-execute.h
> index 50d47b716..8668ab73f 100644
> --- a/lib/odp-execute.h
> +++ b/lib/odp-execute.h
> @@ -23,6 +23,7 @@
>  #include <stdint.h>
>  #include "openvswitch/types.h"
>
> +struct ds;
>  struct nlattr;
>  struct dp_packet;
>  struct pkt_metadata;
> @@ -36,6 +37,7 @@ typedef void (*odp_execute_action_cb)(struct dp_packet_batch *batch,
>                                        const struct nlattr *action);
>
>  int odp_actions_impl_set(const char *name);
> +int odp_actions_impl_get(struct ds *name);
>
>  typedef void (*odp_execute_cb)(void *dp, struct dp_packet_batch *batch,
>                                 const struct nlattr *action, bool should_steal);
> diff --git a/tests/pmd.at b/tests/pmd.at
> index e6b173dab..ac05f5f7d 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -1200,3 +1200,33 @@ ovs-appctl: ovs-vswitchd: server returned an error
>
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([PMD - ovs-actions configuration])
> +OVS_VSWITCHD_START([], [], [], [--dummy-numa 0,0])
> +AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy-pmd])
> +
> +dnl Scalar impl is set by default.
> +AT_CHECK([ovs-vsctl show], [], [stdout])
> +AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "scalar"], [], [dnl
> +  scalar (available: Yes, active: Yes)
> +])
> +
> +AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "autovalidator"], [], [dnl
> +  autovalidator (available: Yes, active: No)
> +])
> +
> +dnl Set the autovalidator impl to active.
> +AT_CHECK([ovs-appctl dpif-netdev/action-impl-set autovalidator], [0], [dnl
> +Action implementation set to autovalidator.
> +])
> +
> +AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "scalar"], [], [dnl
> +  scalar (available: Yes, active: No)
> +])
> +
> +AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "autovalidator"], [], [dnl
> +  autovalidator (available: Yes, active: Yes)
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> -- 
> 2.32.0

The above test was failing due to a major issue in the next patch.
Here is the diff required to make these tests work (I've also added the negative tests I requested in v6):

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 5a35c7ce5..3e688fb8b 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1397,11 +1397,11 @@ action_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
     int err = odp_actions_impl_set(argv[1]);
     if (err) {
         ds_put_format(&reply,
-                      "Error: unknown action implementation, %s, specified!\n",
+                      "Error: unknown action implementation, %s, specified!",
                       argv[1]);
         unixctl_command_reply_error(conn, ds_cstr(&reply));
     } else {
-        ds_put_format(&reply, "Action implementation set to %s.\n", argv[1]);
+        ds_put_format(&reply, "Action implementation set to %s.", argv[1]);
         unixctl_command_reply(conn, ds_cstr(&reply));
     }

diff --git a/tests/pmd.at b/tests/pmd.at
index ac05f5f7d..643fa9285 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -1205,6 +1205,11 @@ AT_SETUP([PMD - ovs-actions configuration])
 OVS_VSWITCHD_START([], [], [], [--dummy-numa 0,0])
 AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy-pmd])

+dnl Set the scalar first, so we always have the scalar impl as Active.
+AT_CHECK([ovs-appctl dpif-netdev/action-impl-set scalar], [0], [dnl
+Action implementation set to scalar.
+])
+
-dnl Scalar impl is set by default.
-AT_CHECK([ovs-vsctl show], [], [stdout])
 AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "scalar"], [], [dnl
@@ -1228,5 +1233,11 @@ AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "autovalidator"], [], [
   autovalidator (available: Yes, active: Yes)
 ])

-OVS_VSWITCHD_STOP
+dnl Set the autovalidator impl to active.
+AT_CHECK([ovs-appctl dpif-netdev/action-impl-set invalid_implementation], [2], [], [dnl
+Error: unknown action implementation, invalid_implementation, specified!
+ovs-appctl: ovs-vswitchd: server returned an error
+])
+
+OVS_VSWITCHD_STOP(["/Failed setting action implementation to invalid_implementation/d"])
 AT_CLEANUP
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 3a25f3035..90ceabd63 100644
--- a/NEWS
+++ b/NEWS
@@ -35,6 +35,9 @@  Post-v2.17.0
    - Userspace datapath:
      * Add actions auto-validator function to compare different actions
        implementations against default implementation.
+     * Add command line option to switch between different actions
+       implementations available at run time.
+
 
 
 v2.17.0 - 17 Feb 2022
diff --git a/lib/dpif-netdev-unixctl.man b/lib/dpif-netdev-unixctl.man
index 8cd847416..81ef7d856 100644
--- a/lib/dpif-netdev-unixctl.man
+++ b/lib/dpif-netdev-unixctl.man
@@ -262,3 +262,11 @@  PMDs in the case where no value is specified.  By default "scalar" is used.
 \fIstudy_cnt\fR defaults to 128 and indicates the number of packets that the
 "study" miniflow implementation must parse before choosing an optimal
 implementation.
+
+.IP "\fBdpif-netdev/action-impl-show\fR
+Lists the actions implementations that are available and highlights the
+currently enabled one.
+.
+.IP "\fBdpif-netdev/action-impl-set\fR \fIaction_impl\fR"
+Sets the action implementation to any available implementation. By default
+"scalar" is used.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 47dd7a1a6..5a35c7ce5 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -63,6 +63,7 @@ 
 #include "netdev-vport.h"
 #include "netlink.h"
 #include "odp-execute.h"
+#include "odp-execute-private.h"
 #include "odp-util.h"
 #include "openvswitch/dynamic-string.h"
 #include "openvswitch/list.h"
@@ -1387,6 +1388,37 @@  error:
     ds_destroy(&reply);
 }
 
+static void
+action_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
+{
+    struct ds reply = DS_EMPTY_INITIALIZER;
+
+    int err = odp_actions_impl_set(argv[1]);
+    if (err) {
+        ds_put_format(&reply,
+                      "Error: unknown action implementation, %s, specified!\n",
+                      argv[1]);
+        unixctl_command_reply_error(conn, ds_cstr(&reply));
+    } else {
+        ds_put_format(&reply, "Action implementation set to %s.\n", argv[1]);
+        unixctl_command_reply(conn, ds_cstr(&reply));
+    }
+
+    ds_destroy(&reply);
+}
+
+static void
+action_impl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
+                const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
+{
+    struct ds reply = DS_EMPTY_INITIALIZER;
+
+    odp_execute_action_get_info(&reply);
+    unixctl_command_reply(conn, ds_cstr(&reply));
+    ds_destroy(&reply);
+}
+
 static void
 dpif_netdev_pmd_rebalance(struct unixctl_conn *conn, int argc,
                           const char *argv[], void *aux OVS_UNUSED)
@@ -1624,6 +1656,12 @@  dpif_netdev_init(void)
     unixctl_command_register("dpif-netdev/miniflow-parser-get", "",
                              0, 0, dpif_miniflow_extract_impl_get,
                              NULL);
+    unixctl_command_register("dpif-netdev/action-impl-set", "name",
+                             1, 1, action_impl_set,
+                             NULL);
+    unixctl_command_register("dpif-netdev/action-impl-show", "",
+                             0, 0, action_impl_show,
+                             NULL);
     return 0;
 }
 
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index 267f32c3e..f8d0896b5 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -68,6 +68,18 @@  odp_execute_action_set(const char *name,
     return -EINVAL;
 }
 
+void
+odp_execute_action_get_info(struct ds *string)
+{
+    ds_put_cstr(string, "Available Actions implementations:\n");
+    for (int i = 0; i < ACTION_IMPL_MAX; i++) {
+        ds_put_format(string, "  %s (available: %s, active: %s)\n",
+                      action_impls[i].name,
+                      action_impls[i].available ? "Yes" : "No",
+                      i == active_action_impl_index ? "Yes" : "No");
+    }
+}
+
 void
 odp_execute_action_init(void)
 {
diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
index d3dc669d1..5322eb8df 100644
--- a/lib/odp-execute-private.h
+++ b/lib/odp-execute-private.h
@@ -85,4 +85,7 @@  int action_autoval_init(struct odp_execute_action_impl *self);
 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 */
diff --git a/lib/odp-execute.h b/lib/odp-execute.h
index 50d47b716..8668ab73f 100644
--- a/lib/odp-execute.h
+++ b/lib/odp-execute.h
@@ -23,6 +23,7 @@ 
 #include <stdint.h>
 #include "openvswitch/types.h"
 
+struct ds;
 struct nlattr;
 struct dp_packet;
 struct pkt_metadata;
@@ -36,6 +37,7 @@  typedef void (*odp_execute_action_cb)(struct dp_packet_batch *batch,
                                       const struct nlattr *action);
 
 int odp_actions_impl_set(const char *name);
+int odp_actions_impl_get(struct ds *name);
 
 typedef void (*odp_execute_cb)(void *dp, struct dp_packet_batch *batch,
                                const struct nlattr *action, bool should_steal);
diff --git a/tests/pmd.at b/tests/pmd.at
index e6b173dab..ac05f5f7d 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -1200,3 +1200,33 @@  ovs-appctl: ovs-vswitchd: server returned an error
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([PMD - ovs-actions configuration])
+OVS_VSWITCHD_START([], [], [], [--dummy-numa 0,0])
+AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy-pmd])
+
+dnl Scalar impl is set by default.
+AT_CHECK([ovs-vsctl show], [], [stdout])
+AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "scalar"], [], [dnl
+  scalar (available: Yes, active: Yes)
+])
+
+AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "autovalidator"], [], [dnl
+  autovalidator (available: Yes, active: No)
+])
+
+dnl Set the autovalidator impl to active.
+AT_CHECK([ovs-appctl dpif-netdev/action-impl-set autovalidator], [0], [dnl
+Action implementation set to autovalidator.
+])
+
+AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "scalar"], [], [dnl
+  scalar (available: Yes, active: No)
+])
+
+AT_CHECK([ovs-appctl dpif-netdev/action-impl-show | grep "autovalidator"], [], [dnl
+  autovalidator (available: Yes, active: Yes)
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP