diff mbox series

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

Message ID 20220510142202.1087967-6-emma.finn@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 fail test: fail

Commit Message

Emma Finn May 10, 2022, 2:21 p.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-get

Added separate test-case for ovs-actions get/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                        |  2 ++
 lib/dpif-netdev-unixctl.man |  6 ++++++
 lib/dpif-netdev.c           | 39 +++++++++++++++++++++++++++++++++++++
 lib/odp-execute-private.c   | 14 +++++++++++++
 lib/odp-execute.h           |  3 +++
 tests/pmd.at                | 21 ++++++++++++++++++++
 6 files changed, 85 insertions(+)

Comments

Eelco Chaudron June 2, 2022, 2:18 p.m. UTC | #1 
On 10 May 2022, at 16:21, 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-get
>
> Added separate test-case for ovs-actions get/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                        |  2 ++
>  lib/dpif-netdev-unixctl.man |  6 ++++++
>  lib/dpif-netdev.c           | 39 +++++++++++++++++++++++++++++++++++++
>  lib/odp-execute-private.c   | 14 +++++++++++++
>  lib/odp-execute.h           |  3 +++
>  tests/pmd.at                | 21 ++++++++++++++++++++
>  6 files changed, 85 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index 8539a03b6..eecdcda5e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -60,6 +60,8 @@ v2.17.0 - 17 Feb 2022
>       * Add support for running threads on cores >= RTE_MAX_LCORE.
>       * 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.
>     - Python:
>       * For SSL support, the use of the pyOpenSSL library has been replaced
>         with the native 'ssl' module.
> diff --git a/lib/dpif-netdev-unixctl.man b/lib/dpif-netdev-unixctl.man
> index 8cd847416..500daf4de 100644
> --- a/lib/dpif-netdev-unixctl.man
> +++ b/lib/dpif-netdev-unixctl.man
> @@ -262,3 +262,9 @@ 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-get\fR
> +Lists the actions implementations that are available.

Maybe enhance it to something like:

Lists actions implementations that are available and highlight the currently enabled one.

> +.
> +.IP "\fBdpif-netdev/action-impl-set\fR \fIaction_impl\fR"

Use action-impl-show instead, see comments below.

> +Sets the action to be used to \fIaction_impl\fR. By default "scalar" is used.

Sets the action implementation to....

> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index f74f8b864..0a6437b0c 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"
> @@ -1398,6 +1399,38 @@ 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;
> +
> +    int32_t err = odp_actions_impl_set(argv[1]);

int

> +    if (err) {
> +        ds_put_format(&reply, "action implementation %s not found.\n",
> +                      argv[1]);

We should keep the same format as in other avx patches for error messages.
So this would be something like:

"Error: unknown action implementation, %s, specified!\n",

> +        const char *reply_str = ds_cstr(&reply);
> +        unixctl_command_reply_error(conn, reply_str);
> +        VLOG_ERR("%s", reply_str);

No need to do the err log here, the user will get direct feedback on the console.
If this is removed you do not need this const char casting, it would become something like:

@@ -1395,11 +1395,10 @@ action_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,

     int32_t err = odp_actions_impl_set(argv[1]);
     if (err) {
-        ds_put_format(&reply, "action implementation %s not found.\n",
+        ds_put_format(&reply,
+                      "Error: unknown action implementation, %s, specified!\n",
                       argv[1]);
-        const char *reply_str = ds_cstr(&reply);
-        unixctl_command_reply_error(conn, reply_str);
-        VLOG_ERR("%s", reply_str);
+        unixctl_command_reply_error(conn, ds_cstr(&reply));
         ds_destroy(&reply);
         return;
     }

> +        ds_destroy(&reply);
> +        return;
> +    }

What about making this an else {}, and making the ds_destory common?

> +
> +    ds_put_format(&reply, "action implementation set to %s.\n", argv[1]);

Capital A for Action.

> +    unixctl_command_reply(conn, ds_cstr(&reply));
> +    ds_destroy(&reply);
> +}
> +
> +static void
> +action_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,

Rename this to action_impl_show, see comments below.

> +                const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +{
> +    struct ds reply = DS_EMPTY_INITIALIZER;

Add an empty line here.

> +    odp_execute_action_get(&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)
> @@ -1635,6 +1668,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-get", "",
> +                             0, 0, action_impl_get,

Here we should rename the command to dpif-netdev/action-impl-show, see discussion on other reviews.

The -get would imply you would get a (single) configured value, however, here we show all implementations, which ones are available for this systems ISA, and also the used one.

Also, change the action_impl_get() to action_impl_show() to be inline.

> +                             NULL);
>      return 0;
>  }
>
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> index e85aed636..8f5f8723f 100644
> --- a/lib/odp-execute-private.c
> +++ b/lib/odp-execute-private.c
> @@ -81,6 +81,20 @@ odp_execute_action_set(const char *name,
>      return -1;
>  }
>
> +void
> +odp_execute_action_get(struct ds *string)

Maybe making this odp_execute_action_get_info(), so we know what we are getting?

> +{
> +    uint32_t i;

int

> +
> +    ds_put_cstr(string, "Available Actions implementations:\n");
> +    for (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 ? "True" : "False",
> +                      i == active_action_impl_index ? "True" : "False");

I'm not a big fan of true/false this is more a programmer’s terminology than an end-user.
Other configurations use, yes / no, I would prefer it here too.

> +    }
> +}
> +
>  void
>  odp_execute_action_init(void)
>  {
> diff --git a/lib/odp-execute.h b/lib/odp-execute.h
> index 6441392b9..4f4cdc4ac 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;
> @@ -32,6 +33,8 @@ struct dp_packet_batch;
>  /* Called once at initialization time. */
>  void odp_execute_init(void);
>
> +/* Runtime update get/set functionality. */
> +int32_t odp_actions_impl_get(struct ds *name);
>  int32_t odp_actions_impl_set(const char *name);

Both should have been changed to int, see comments in earlier patches.

>
>  typedef void (*odp_execute_cb)(void *dp, struct dp_packet_batch *batch,
> diff --git a/tests/pmd.at b/tests/pmd.at
> index 0a451f33c..65b4376e4 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -1200,3 +1200,24 @@ 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 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.
> +])
> +
> +AT_CHECK([ovs-vsctl show], [], [stdout])
> +AT_CHECK([ovs-appctl dpif-netdev/action-impl-get | grep "scalar"], [], [dnl
> +  scalar (available: True, active: True)
> +])
> +
> +AT_CHECK([ovs-appctl dpif-netdev/action-impl-get | grep "autovalidator"], [], [dnl
> +  autovalidator (available: True, active: False)
> +])

- The test case should also try to set the autovalidator and see if it’s set.
- Also it should try a negative test, i.e. set the test to scalars for example.

> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> \ No newline at end of file
> -- 
> 2.25.1
diff mbox series

Patch

diff --git a/NEWS b/NEWS
index 8539a03b6..eecdcda5e 100644
--- a/NEWS
+++ b/NEWS
@@ -60,6 +60,8 @@  v2.17.0 - 17 Feb 2022
      * Add support for running threads on cores >= RTE_MAX_LCORE.
      * 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.
    - Python:
      * For SSL support, the use of the pyOpenSSL library has been replaced
        with the native 'ssl' module.
diff --git a/lib/dpif-netdev-unixctl.man b/lib/dpif-netdev-unixctl.man
index 8cd847416..500daf4de 100644
--- a/lib/dpif-netdev-unixctl.man
+++ b/lib/dpif-netdev-unixctl.man
@@ -262,3 +262,9 @@  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-get\fR
+Lists the actions implementations that are available.
+.
+.IP "\fBdpif-netdev/action-impl-set\fR \fIaction_impl\fR"
+Sets the action to be used to \fIaction_impl\fR. By default "scalar" is used.
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index f74f8b864..0a6437b0c 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"
@@ -1398,6 +1399,38 @@  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;
+
+    int32_t err = odp_actions_impl_set(argv[1]);
+    if (err) {
+        ds_put_format(&reply, "action implementation %s not found.\n",
+                      argv[1]);
+        const char *reply_str = ds_cstr(&reply);
+        unixctl_command_reply_error(conn, reply_str);
+        VLOG_ERR("%s", reply_str);
+        ds_destroy(&reply);
+        return;
+    }
+
+    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_get(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(&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)
@@ -1635,6 +1668,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-get", "",
+                             0, 0, action_impl_get,
+                             NULL);
     return 0;
 }
 
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index e85aed636..8f5f8723f 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -81,6 +81,20 @@  odp_execute_action_set(const char *name,
     return -1;
 }
 
+void
+odp_execute_action_get(struct ds *string)
+{
+    uint32_t i;
+
+    ds_put_cstr(string, "Available Actions implementations:\n");
+    for (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 ? "True" : "False",
+                      i == active_action_impl_index ? "True" : "False");
+    }
+}
+
 void
 odp_execute_action_init(void)
 {
diff --git a/lib/odp-execute.h b/lib/odp-execute.h
index 6441392b9..4f4cdc4ac 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;
@@ -32,6 +33,8 @@  struct dp_packet_batch;
 /* Called once at initialization time. */
 void odp_execute_init(void);
 
+/* Runtime update get/set functionality. */
+int32_t odp_actions_impl_get(struct ds *name);
 int32_t odp_actions_impl_set(const char *name);
 
 typedef void (*odp_execute_cb)(void *dp, struct dp_packet_batch *batch,
diff --git a/tests/pmd.at b/tests/pmd.at
index 0a451f33c..65b4376e4 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -1200,3 +1200,24 @@  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 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.
+])
+
+AT_CHECK([ovs-vsctl show], [], [stdout])
+AT_CHECK([ovs-appctl dpif-netdev/action-impl-get | grep "scalar"], [], [dnl
+  scalar (available: True, active: True)
+])
+
+AT_CHECK([ovs-appctl dpif-netdev/action-impl-get | grep "autovalidator"], [], [dnl
+  autovalidator (available: True, active: False)
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
\ No newline at end of file