On 13 Jan 2026, at 14:23, Ilya Maximets wrote:
> On 1/12/26 12:20 PM, Eelco Chaudron wrote: >> 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 <elibr.nvidia.com> >> 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_(). >> --- >> lib/dpif-offload.c | 124 ++++++++++++++++++++++++++++++++++------ >> 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, 175 insertions(+), 20 deletions(-) >> >> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c >> index 80c8bf1f9..d65a887f7 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. */ > > nit: It's better to be less personal in comments, e.g. "While adding a new > offload class to this structure make sure to also update ... " Done. >> &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 @@ dp_offload_initialize(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); > > This more for the first patch, but I wonder if it's better to call this > 'providers' instead of 'classes'... left it as classes for now, since it shows the globally available dpif_offload_class instances, not providers assigned to a specific dpif. That said, I’m fine either way, just let me know your preference, and I’ll change it. >> @@ -193,13 +204,37 @@ dpif_offload_attach_dp_offload(struct dpif *dpif, >> ovsrcu_set(&dpif->dp_offload, dp_offload); >> } >> >> +static void >> +dpif_offload_attach_providers__(struct dpif *dpif, >> + struct dp_offload *dp_offload, >> + struct dpif_offload *offload) > > This function attaches a single provider, so it should not have > plural form in the name. In line with the previous naming suggestions, > it should be something like: > dpif_offload_attach_one_provider(). I renamed this already when doing the rework, called it move_provider_to_collection() > I'm also not sure if we need to pass dpif pointer here, can we use > the dpif_name stored in the dp_offload instead? Good catch! Guess too much refactoring went on here :) >> + OVS_REQUIRES(dpif_offload_mutex) >> +{ >> + 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); >> + } >> +} >> + >> 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); >> @@ -209,37 +244,51 @@ 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; >> >> 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 = 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); >> + >> + 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) { >> + if (strcmp(name, offload->class->type)) { >> + continue; >> + } >> + >> + dpif_offload_attach_providers__(dpif, dp_offload, offload); >> + break; >> + } >> + } >> + free(tokens); >> + >> + /* Add remaining entries in order. */ >> + LIST_FOR_EACH_SAFE (offload, dpif_list_node, &provider_list) { >> + dpif_offload_attach_providers__(dpif, dp_offload, offload); >> + } >> + >> /* Attach dp_offload to dpif. */ >> ovsrcu_set(&dpif->dp_offload, dp_offload); >> >> @@ -401,3 +450,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 11f9e346a..1d80fc439 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 >> @@ -10122,7 +10157,7 @@ AT_CHECK([ovs-appctl --format json --pretty >> dpif/offload/show], [0], [dnl >> "dummy"]]}} >> ]) >> >> -OVS_VSWITCHD_STOP >> +OVS_TRAFFIC_VSWITCHD_STOP > > This doesn't look right. Ack, replace with the normal OVS_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']]) > > This is wild. Maybe just don't quote the name in the log? Yes, this took me a while to figure out ;) But you’re right, I removed the quote from the log message. >> ]) >> >> # 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..e235f7e79 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 > > Stray 'The'. Ack. >> + 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 > > Double space. But also, we probably shouldn't document the dummy provider > names. > They are for internal testing. Added space, and removed dummy providers. >> + 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
