Message ID | 20220713182807.3416578-5-harry.van.haaren@intel.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Actions Infrastructure + Optimizations | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
ovsrobot/intel-ovs-compilation | fail | test: fail |
Hey all,
Have tested the all the patches in the series.
CI fails randomly again due to ubuntu download dependency/ random fails not related to patch.
<Snip>
Tested-by: Kumar Amber <kumar.amber@intel.com>
BR
Amber
Hi Harry/Emma, > -----Original Message----- > From: Van Haaren, Harry <harry.van.haaren@intel.com> > Sent: Wednesday, July 13, 2022 11:58 PM > To: 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>; Van Haaren, > Harry <harry.van.haaren@intel.com> > Subject: [PATCH v10 04/10] odp-execute: Add command to switch action > implementation. > > From: Emma Finn <emma.finn@intel.com> > > This commit adds a new command to allow the user to switch the active > action implementation at runtime. > > Usage: > $ ovs-appctl odp-execute/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 odp-execute/action-impl-show > > Added separate test-case for ovs-actions show/set commands: > PMD - ovs-actions configuration > > Signed-off-by: Emma Finn <emma.finn@intel.com> > Signed-off-by: Kumar Amber <kumar.amber@intel.com> > Signed-off-by: Sunil Pai G <sunil.pai.g@intel.com> > Co-authored-by: Kumar Amber <kumar.amber@intel.com> > Co-authored-by: Sunil Pai G <sunil.pai.g@intel.com> > Acked-by: Harry van Haaren <harry.van.haaren@intel.com> Like patch 1, GHA couldn't complete its run for this patch because of infra reasons, so I manually triggered one on my forked repo to be more confident, and it succeeds: https://github.com/Sunil-Pai-G/ovs/actions/runs/2668395147 The patch looks good to me as well, so Acked-by: Sunil Pai G <sunil.pai.g@intel.com> Thanks and regards Sunil Pai G
On 13 Jul 2022, at 20:28, Harry van Haaren wrote: > From: Emma Finn <emma.finn@intel.com> > > This commit adds a new command to allow the user to switch > the active action implementation at runtime. > > Usage: > $ ovs-appctl odp-execute/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 odp-execute/action-impl-show > > Added separate test-case for ovs-actions show/set commands: > PMD - ovs-actions configuration > > Signed-off-by: Emma Finn <emma.finn@intel.com> > Signed-off-by: Kumar Amber <kumar.amber@intel.com> > Signed-off-by: Sunil Pai G <sunil.pai.g@intel.com> > Co-authored-by: Kumar Amber <kumar.amber@intel.com> > Co-authored-by: Sunil Pai G <sunil.pai.g@intel.com> > Acked-by: Harry van Haaren <harry.van.haaren@intel.com> > > --- <SNIP> > @@ -879,6 +880,48 @@ odp_actions_impl_set(const char *name) > > } > > +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", On the v7 I asked to remove the trailing \n, any reason why this was not done? > + argv[1]); > + unixctl_command_reply_error(conn, ds_cstr(&reply)); > + } else { > + ds_put_format(&reply, "Action implementation set to %s.\n", argv[1]); Same as above. > + unixctl_command_reply(conn, ds_cstr(&reply)); > + } > + > + ds_destroy(&reply); <SNIP> > +++ b/tests/pmd.at > @@ -1192,3 +1192,42 @@ 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]) > + These tests are not PMD related, so I think we should move them to odp.at (and change the banner)? Ilya any thoughts? <SNIP> Everything else looks good to me. //Eelco
On 7/14/22 15:20, Eelco Chaudron wrote: > On 13 Jul 2022, at 20:28, Harry van Haaren wrote: > >> +++ b/tests/pmd.at >> @@ -1192,3 +1192,42 @@ 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]) >> + > > These tests are not PMD related, so I think we should move them to odp.at (and change the banner)? Ilya any thoughts? I agree. The test also doesn't need the dummy numa nor any ports added. It can be added to odp.at with: AT_BANNER([datapath actions in userspace]) AT_SETUP([odp-execute - actions implementation]) or something like that. Best regards, Ilya Maximets.
diff --git a/NEWS b/NEWS index 1ef1175d0..d02733936 100644 --- a/NEWS +++ b/NEWS @@ -52,6 +52,8 @@ Post-v2.17.0 The old variant is kept for backward compatibility. * 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. - Linux datapath: * Add offloading meter tc police. * Add support for offloading the check_pkt_len action. diff --git a/lib/automake.mk b/lib/automake.mk index 23ba4fab0..5c3b05f6b 100644 --- a/lib/automake.mk +++ b/lib/automake.mk @@ -584,6 +584,7 @@ MAN_FRAGMENTS += \ lib/netdev-dpdk-unixctl.man \ lib/dpif-netdev-unixctl.man \ lib/dpif-netlink-unixctl.man \ + lib/odp-execute-unixctl.man \ lib/ofp-version.man \ lib/ovs.tmac \ lib/ovs-replay.man \ diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c index 780d6d289..38be22ec9 100644 --- a/lib/odp-execute-private.c +++ b/lib/odp-execute-private.c @@ -67,6 +67,18 @@ odp_execute_action_set(const char *name) return NULL; } +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 074a8d67e..d6eebbf37 100644 --- a/lib/odp-execute-private.h +++ b/lib/odp-execute-private.h @@ -84,4 +84,6 @@ struct odp_execute_action_impl * odp_execute_action_set(const char *name); int action_autoval_init(struct odp_execute_action_impl *self); +void odp_execute_action_get_info(struct ds *name); + #endif /* ODP_EXTRACT_PRIVATE */ diff --git a/lib/odp-execute-unixctl.man b/lib/odp-execute-unixctl.man new file mode 100644 index 000000000..82d51e1d3 --- /dev/null +++ b/lib/odp-execute-unixctl.man @@ -0,0 +1,10 @@ +.SS "ODP-EXECUTE COMMANDS" +These commands manage the "odp-execute" component. + +.IP "\fBodp-execute/action-impl-show\fR +Lists the actions implementations that are available and highlights the +currently enabled one. +. +.IP "\fBodp-execute/action-impl-set\fR \fIaction_impl\fR" +Sets the action implementation to any available implementation. By default +"scalar" is used. diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 368876f27..d5be190e0 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -39,6 +39,7 @@ #include "csum.h" #include "conntrack.h" #include "openvswitch/vlog.h" +#include "unixctl.h" VLOG_DEFINE_THIS_MODULE(odp_execute); COVERAGE_DEFINE(datapath_drop_sample_error); @@ -879,6 +880,48 @@ odp_actions_impl_set(const char *name) } +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 +odp_execute_unixctl_init(void) +{ + unixctl_command_register("odp-execute/action-impl-set", "name", + 1, 1, action_impl_set, + NULL); + unixctl_command_register("odp-execute/action-impl-show", "", + 0, 0, action_impl_show, + NULL); +} + void odp_execute_init(void) { @@ -886,6 +929,7 @@ odp_execute_init(void) if (ovsthread_once_start(&once)) { odp_execute_action_init(); odp_actions_impl_set("scalar"); + odp_execute_unixctl_init(); ovsthread_once_done(&once); } } diff --git a/tests/pmd.at b/tests/pmd.at index 4342c50e0..4ca926581 100644 --- a/tests/pmd.at +++ b/tests/pmd.at @@ -1192,3 +1192,42 @@ 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]) + +AT_CHECK([ovs-vsctl show], [], [stdout]) + +dnl Set the scalar first, so we always have the scalar impl as Active. +AT_CHECK([ovs-appctl odp-execute/action-impl-set scalar], [0], [dnl +Action implementation set to scalar. +]) +AT_CHECK([ovs-appctl odp-execute/action-impl-show | grep "scalar"], [], [dnl + scalar (available: Yes, active: Yes) +]) + +AT_CHECK([ovs-appctl odp-execute/action-impl-show | grep "autovalidator"], [], [dnl + autovalidator (available: Yes, active: No) +]) + +dnl Set the autovalidator impl to active. +AT_CHECK([ovs-appctl odp-execute/action-impl-set autovalidator], [0], [dnl +Action implementation set to autovalidator. +]) + +AT_CHECK([ovs-appctl odp-execute/action-impl-show | grep "scalar"], [], [dnl + scalar (available: Yes, active: No) +]) + +AT_CHECK([ovs-appctl odp-execute/action-impl-show | grep "autovalidator"], [], [dnl + autovalidator (available: Yes, active: Yes) +]) + +AT_CHECK([ovs-appctl odp-execute/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 --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in index 1a32402be..9569265fc 100644 --- a/vswitchd/ovs-vswitchd.8.in +++ b/vswitchd/ovs-vswitchd.8.in @@ -282,6 +282,7 @@ type). .so lib/dpif-netdev-unixctl.man .so lib/dpif-netlink-unixctl.man .so lib/netdev-dpdk-unixctl.man +.so lib/odp-execute-unixctl.man .so ofproto/ofproto-dpif-unixctl.man .so ofproto/ofproto-unixctl.man .so lib/vlog-unixctl.man