This patch adds support for configuring the priority of offload providers. When multiple providers exist and support the same port, the 'hw-offload-priority' option allows specifying the order in which they are tried.
Acked-by: Eli Britstein <[email protected]> Signed-off-by: Eelco Chaudron <[email protected]> --- v2 changes: - Fixed a memory leak with the 'dpif_offload_provider_priority_list' variable. v3 changes: - Fixed some new line style issues. - Removed duplicate code in dpif_offload_attach_providers_(). v6 changes: - Addressed Ilya's comments, mainly some small nits, log message content, and some spelling problems. --- lib/dpif-offload.c | 141 +++++++++++++++++++++++++++++++++------- lib/dpif-offload.h | 4 ++ tests/ofproto-dpif.at | 35 ++++++++++ tests/ofproto-macros.at | 9 ++- vswitchd/bridge.c | 2 + vswitchd/vswitch.xml | 19 ++++++ 6 files changed, 182 insertions(+), 28 deletions(-) diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c index f073d4df6..d6a578ed0 100644 --- a/lib/dpif-offload.c +++ b/lib/dpif-offload.c @@ -43,10 +43,16 @@ static const struct dpif_offload_class *base_dpif_offload_classes[] = { #ifdef DPDK_NETDEV &dpif_offload_dpdk_class, #endif + /* While adding a new offload class to this structure make sure to also + * update the dpif_offload_provider_priority_list below. */ &dpif_offload_dummy_class, &dpif_offload_dummy_x_class, }; +#define DEFAULT_PROVIDER_PRIORITY_LIST "tc,dpdk,dummy,dummy_x" + +static char *dpif_offload_provider_priority_list = NULL; + static int dpif_offload_register_provider__(const struct dpif_offload_class *class) OVS_REQUIRES(dpif_offload_mutex) @@ -131,6 +137,11 @@ dpif_offload_module_init(void) return; } + if (!dpif_offload_provider_priority_list) { + dpif_offload_provider_priority_list = xstrdup( + DEFAULT_PROVIDER_PRIORITY_LIST); + } + unixctl_command_register("dpif/offload/classes", NULL, 0, 0, dpif_offload_show_classes, NULL); @@ -151,6 +162,19 @@ dpif_get_offload_provider_collection(const struct dpif *dpif) &dpif->offload_provider_collection); } +static void +dpif_attach_offload_provider_collection( + struct dpif *dpif, struct dpif_offload_provider_collection *collection) + OVS_REQUIRES(dpif_offload_mutex) +{ + /* When called, 'collection' should still have a refcount > 0, which is + * guaranteed by holding the lock from the shash lookup up to this point. + * If, for any reason, the refcount is not > 0, ovs_refcount_ref() will + * assert. */ + ovs_refcount_ref(&collection->ref_cnt); + ovsrcu_set(&dpif->offload_provider_collection, collection); +} + static int provider_collection_add( struct dpif_offload_provider_collection *collection, struct dpif_offload *offload) @@ -172,25 +196,34 @@ static int provider_collection_add( } static void -dpif_attach_offload_provider_collection( - struct dpif *dpif, struct dpif_offload_provider_collection *collection) - OVS_REQUIRES(dpif_offload_mutex) +move_provider_to_collection( + struct dpif_offload_provider_collection *collection, + struct dpif_offload *offload) OVS_REQUIRES(dpif_offload_mutex) { - /* When called, 'collection' should still have a refcount > 0, which is - * guaranteed by holding the lock from the shash lookup up to this point. - * If, for any reason, the refcount is not > 0, ovs_refcount_ref() will - * assert. */ - ovs_refcount_ref(&collection->ref_cnt); - ovsrcu_set(&dpif->offload_provider_collection, collection); + int error; + + ovs_list_remove(&offload->dpif_list_node); + error = provider_collection_add(collection, offload); + if (error) { + VLOG_WARN( + "failed to add dpif offload provider %s to %s: %s", + dpif_offload_type(offload), collection->dpif_name, + ovs_strerror(error)); + + offload->class->close(offload); + } } static int dpif_attach_new_offload_provider_collection(struct dpif *dpif) OVS_REQUIRES(dpif_offload_mutex) { + struct ovs_list provider_list = OVS_LIST_INITIALIZER(&provider_list); const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif)); struct dpif_offload_provider_collection *collection; + struct dpif_offload *offload; struct shash_node *node; + char *tokens, *saveptr; /* Allocate and attach collection to dpif. */ collection = xmalloc(sizeof *collection); @@ -200,37 +233,52 @@ dpif_attach_new_offload_provider_collection(struct dpif *dpif) ovs_list_init(&collection->list); shash_add(&dpif_offload_providers, collection->dpif_name, collection); - /* Attach all the providers supporting this dpif type. */ + /* Open all the providers supporting this dpif type. */ SHASH_FOR_EACH (node, &dpif_offload_classes) { const struct dpif_offload_class *class = node->data; for (size_t i = 0; class->supported_dpif_types[i] != NULL; i++) { if (!strcmp(class->supported_dpif_types[i], dpif_type_str)) { - struct dpif_offload *offload; - int error; - - error = class->open(class, dpif, &offload); - if (!error) { - error = provider_collection_add(collection, offload); - if (error) { - VLOG_WARN("failed to add dpif offload provider " - "%s to %s: %s", - class->type, dpif_name(dpif), - ovs_strerror(error)); - class->close(offload); - } - } else { + int error = class->open(class, dpif, &offload); + + if (error) { VLOG_WARN("failed to initialize dpif offload provider " "%s for %s: %s", class->type, dpif_name(dpif), ovs_strerror(error)); + } else { + ovs_list_push_back(&provider_list, + &offload->dpif_list_node); } break; } } } - /* Attach offload collection to dpif. */ + /* Attach all the providers based on the priority list. */ + tokens = xstrdup(dpif_offload_provider_priority_list); + + for (char *name = strtok_r(tokens, ",", &saveptr); + name; + name = strtok_r(NULL, ",", &saveptr)) { + + LIST_FOR_EACH_SAFE (offload, dpif_list_node, &provider_list) { + if (strcmp(name, offload->class->type)) { + continue; + } + + move_provider_to_collection(collection, offload); + break; + } + } + free(tokens); + + /* Add remaining entries in order. */ + LIST_FOR_EACH_SAFE (offload, dpif_list_node, &provider_list) { + move_provider_to_collection(collection, offload); + } + + /* Attach the new collection to the dpif. */ ovsrcu_set(&dpif->offload_provider_collection, collection); return 0; @@ -397,3 +445,46 @@ dpif_offload_dump_done(struct dpif_offload_dump *dump) { return dump->error == EOF ? 0 : dump->error; } + +void +dpif_offload_set_global_cfg(const struct smap *other_cfg) +{ + static struct ovsthread_once init_once = OVSTHREAD_ONCE_INITIALIZER; + const char *priority = smap_get(other_cfg, "hw-offload-priority"); + + /* The 'hw-offload-priority' parameter can only be set at startup, + * any successive change needs a restart. */ + if (ovsthread_once_start(&init_once)) { + /* Initialize the dpif-offload layer in case it's not yet initialized + * at the first invocation of setting the configuration. */ + dpif_offload_module_init(); + + /* If priority is not set keep the default value. */ + if (priority) { + char *tokens = xstrdup(priority); + char *saveptr; + + free(dpif_offload_provider_priority_list); + dpif_offload_provider_priority_list = xstrdup(priority); + + /* Log a warning for unknown offload providers. */ + for (char *name = strtok_r(tokens, ",", &saveptr); + name; + name = strtok_r(NULL, ",", &saveptr)) { + + if (!shash_find(&dpif_offload_classes, name)) { + VLOG_WARN("hw-offload-priority configuration has an " + "unknown type; %s", name); + } + } + free(tokens); + } + ovsthread_once_done(&init_once); + } else { + if (priority && strcmp(priority, + dpif_offload_provider_priority_list)) { + VLOG_INFO_ONCE("hw-offload-priority configuration changed; " + "restart required"); + } + } +} diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h index 78363ac8d..ec003b639 100644 --- a/lib/dpif-offload.h +++ b/lib/dpif-offload.h @@ -30,6 +30,10 @@ struct dpif_offload_dump { void *state; }; + +/* Global functions. */ +void dpif_offload_set_global_cfg(const struct smap *other_cfg); + /* Per dpif specific functions. */ void dpif_offload_init(struct dpif_offload *, diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 11f9e346a..b0238c5e9 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -10108,6 +10108,41 @@ AT_SETUP([ofproto-dpif - offload - ovs-appctl dpif/offload/]) AT_KEYWORDS([dpif-offload]) OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy]) +AT_CHECK([ovs-appctl dpif/offload/show], [0], [dnl +dummy@ovs-dummy: + dummy + dummy_x +]) + +AT_CHECK([ovs-appctl --format json --pretty dpif/offload/show], [0], [dnl +{ + "dummy@ovs-dummy": { + "providers": [[ + "dummy", + "dummy_x"]]}} +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([ofproto-dpif - offload - priority order - warning]) +AT_KEYWORDS([dpif-offload]) +OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy], [], [], + [], [-- set Open_vSwitch . other_config:hw-offload-priority=none,dummy]) + +OVS_WAIT_UNTIL( + [grep "hw-offload-priority configuration has an unknown type; none" \ + ovs-vswitchd.log]) + +OVS_VSWITCHD_STOP( + ["/hw-offload-priority configuration has an unknown type;/d"]) +AT_CLEANUP + +AT_SETUP([ofproto-dpif - offload - priority order]) +AT_KEYWORDS([dpif-offload]) +OVS_VSWITCHD_START([add-br br1 -- set bridge br1 datapath-type=dummy], [], [], + [], [-- set Open_vSwitch . other_config:hw-offload-priority=dummy_x,dummy]) + AT_CHECK([ovs-appctl dpif/offload/show], [0], [dnl dummy@ovs-dummy: dummy_x diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index 3cd49d7a7..d3c5349ef 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -227,11 +227,12 @@ m4_define([_OVS_VSWITCHD_START], /netdev_offload|INFO|netdev: Flow API Enabled/d /probe tc:/d /setting extended ack support failed/d -/tc: Using policy/d']]) +/tc: Using policy/d +/hw-offload-priority configuration has an unknown type;/d']]) ]) # OVS_VSWITCHD_START([vsctl-args], [vsctl-output], [=override], -# [vswitchd-aux-args]) +# [vswitchd-aux-args] [dbinit-aux-args]) # # Creates a database and starts ovsdb-server, starts ovs-vswitchd # connected to that database, calls ovs-vsctl to create a bridge named @@ -248,7 +249,9 @@ m4_define([_OVS_VSWITCHD_START], # 'vswitchd-aux-args' provides a way to pass extra command line arguments # to ovs-vswitchd m4_define([OVS_VSWITCHD_START], - [_OVS_VSWITCHD_START([--enable-dummy$3 --disable-system --disable-system-route $4]) + [_OVS_VSWITCHD_START( + [--enable-dummy$3 --disable-system --disable-system-route $4], + [$5]) AT_CHECK([add_of_br 0 $1 m4_if([$2], [], [], [| uuidfilt])], [0], [$2]) ]) diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 475eefefa..62dec1391 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -28,6 +28,7 @@ #include "daemon.h" #include "dirs.h" #include "dpif.h" +#include "dpif-offload.h" #include "dpdk.h" #include "hash.h" #include "openvswitch/hmap.h" @@ -3395,6 +3396,7 @@ bridge_run(void) cfg = ovsrec_open_vswitch_first(idl); if (cfg) { + dpif_offload_set_global_cfg(&cfg->other_config); netdev_set_flow_api_enabled(&cfg->other_config); dpdk_init(&cfg->other_config); userspace_tso_init(&cfg->other_config); diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index b93ad9344..370b3c296 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -254,6 +254,25 @@ </p> </column> + <column name="other_config" key="hw-offload-priority" + type='{"type": "string"}'> + <p> + This configuration sets the order of the hardware offload providers + to try when multiple exist for a given datapath implementation. + The argument provided should be a list of comma-separated hardware + offload provider names. + </p> + <p> + If implementations are missing from the list, they are added at the + end in undefined order. + </p> + <p> + The default value is <code>tc,dpdk</code>. Changing this value + requires restarting the daemon if hardware offload is already + enabled. + </p> + </column> + <column name="other_config" key="n-offload-threads" type='{"type": "integer", "minInteger": 1, "maxInteger": 10}'> <p> -- 2.52.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
