On 8 Dec 2025, at 19:21, Eli Britstein wrote:
> 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. I will update my working branch with your suggestions, and include the changes in the next revision. Let me know once you are done, so I can send a v3 if there are no other review comments. Thanks, Eelco > 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. Yes, but I guess this code/layer should handle this independently. I’ll move it to the first patch. >> + >> + 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. Ack >> 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. Ack >> - 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. Ack >> + 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. Ack >> + if (!strcmp(name, offload->class->type)) { > Suggestion (to easier indentation): > If (strcmp()) { > Continue; > } > ... Good catch! >> + 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. Done >> + } >> + >> /* 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
