Eelco, I started a review. So far I just see nits here. Could you please check those and maybe other patches with the same things?
I will look for the other commits. I just wanted to send some preliminary comments. Thanks, Eli >-----Original Message----- >From: dev <[email protected]> On Behalf Of Eelco Chaudron >Sent: Tuesday, 2 December 2025 16:05 >To: [email protected] >Subject: [ovs-dev] [PATCH v2 04/41] dpif-offload: Allow configuration of >offload >provider priority. > >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. > >Signed-off-by: Eelco Chaudron <[email protected]> >--- > >v2 changes: > - Fixed a memory leak with the 'dpif_offload_provider_priority_list' > variable. >--- > lib/dpif-offload.c | 133 +++++++++++++++++++++++++++++++++++----- > lib/dpif-offload.h | 4 ++ > tests/ofproto-dpif.at | 37 ++++++++++- > tests/ofproto-macros.at | 9 ++- > vswitchd/bridge.c | 2 + > vswitchd/vswitch.xml | 19 ++++++ > 6 files changed, 183 insertions(+), 21 deletions(-) > >diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c index ab87ed2fe..c107735fe >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 >+ /* If you add an offload class to this structure, make sure you 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) >@@ -125,6 +131,17 @@ dpif_offload_show_classes(struct unixctl_conn *conn, >int argc OVS_UNUSED, void > dp_offload_initialize(void) > { >+ static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >+ >+ if (!ovsthread_once_start(&once)) { >+ return; >+ } The code in this function does not require "once" since it is called from dpif.c under another "once" there. However, you can have it, but move it to the first commit introduced it. >+ >+ 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); > >@@ -134,6 +151,7 @@ dp_offload_initialize(void) > > dpif_offload_register_provider(base_dpif_offload_classes[i]); > } >+ ovsthread_once_done(&once); > } > > static struct dp_offload* >@@ -186,9 +204,12 @@ static int > dpif_offload_attach_providers_(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 dp_offload *dp_offload; >+ struct dpif_offload *offload; > struct shash_node *node; >+ char *tokens, *saveptr; > > /* Allocate and attach dp_offload to dpif. */ > dp_offload = xmalloc(sizeof *dp_offload); @@ -198,37 +219,72 @@ >dpif_offload_attach_providers_(struct dpif *dpif) > ovs_list_init(&dp_offload->offload_providers); > shash_add(&dpif_offload_providers, dp_offload->dpif_name, dp_offload); > >- /* 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; >+ Move this blank line to the first commit introduced this code. > 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) { >- This blank line (in previous commit) should be omitted. >- error = dpif_offload_attach_provider_to_dp_offload( >- dp_offload, 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); Add blank line. >+ 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 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) { >+ Omit blank line. >+ if (!strcmp(name, offload->class->type)) { Suggestion (to easier indentation): If (strcmp()) { Continue; } ... >+ int error; >+ >+ ovs_list_remove(&offload->dpif_list_node); >+ error = dpif_offload_attach_provider_to_dp_offload(dp_offload, >+ offload); >+ if (error) { >+ VLOG_WARN( >+ "failed to add dpif offload provider %s to %s: %s", >+ offload->class->type, dpif_name(dpif), >+ ovs_strerror(error)); >+ >+ offload->class->close(offload); >+ } >+ break; >+ } >+ } >+ } >+ free(tokens); >+ >+ /* Add remaining entries in order. */ >+ LIST_FOR_EACH_SAFE (offload, dpif_list_node, &provider_list) { >+ int error; >+ >+ ovs_list_remove(&offload->dpif_list_node); >+ error = dpif_offload_attach_provider_to_dp_offload(dp_offload, >+ offload); >+ if (error) { >+ VLOG_WARN("failed to add dpif offload provider %s to %s: %s", >+ offload->class->type, dpif_name(dpif), >+ ovs_strerror(error)); >+ >+ offload->class->close(offload); >+ } The code here is duplicated as above. Put it in a helper function and call from both places. >+ } >+ > /* Attach dp_offload to dpif. */ > ovsrcu_set(&dpif->dp_offload, dp_offload); > >@@ -391,3 +447,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. */ >+ dp_offload_initialize(); >+ >+ /* 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 d61d8e0bf..43d8b6b74 >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 65951bcd3..ef01c42af 100644 >--- a/tests/ofproto-dpif.at >+++ b/tests/ofproto-dpif.at >@@ -10079,6 +10079,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 >@@ -10093,7 +10128,7 @@ AT_CHECK([ovs-appctl --format json --pretty >dpif/offload/show], [0], [dnl > "dummy"]]}} > ]) > >-OVS_VSWITCHD_STOP >+OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > dnl ---------------------------------------------------------------------- >diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index >3cd49d7a7..71cd2aed0 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 >3dcf74402..061d0a568 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 >+ 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,dummy,dummy_x</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> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
