On 13 Jan 2026, at 14:56, Ilya Maximets wrote:
> On 1/12/26 12:20 PM, Eelco Chaudron wrote: >> This patch introduces an API that allows offload providers to >> manage global configurations. It also moves the 'n-offload-threads' >> and 'tc-policy' setting to the appropriate providers using >> this new API. >> >> We maintain the existing behavior where global configurations >> are only accepted when hardware offload is initially enabled. >> Once enabled, they cannot be updated or reconfigured. >> >> Acked-by: Eli Britstein <elibr.nvidia.com> >> Signed-off-by: Eelco Chaudron <[email protected]> Hi Ilya, Thanks for taking the time to go over this monster series. See inline ACKs below. One small thing I need clarification on: the dpif_offload_class argument. //Eelco >> --- >> v5 changes: >> - Updated commit message, and removed confusing comments. >> --- >> lib/dpif-offload-dpdk.c | 79 ++++++++++++++++++++++++++++++++++--- >> lib/dpif-offload-provider.h | 17 +++++++- >> lib/dpif-offload-tc.c | 49 +++++++++++++++++++++-- >> lib/dpif-offload.c | 18 +++++++++ >> lib/dpif.c | 1 + >> lib/netdev-offload.c | 37 ++++++----------- >> tests/dpif-netdev.at | 6 +-- >> tests/ofproto-macros.at | 2 +- >> 8 files changed, 170 insertions(+), 39 deletions(-) >> >> diff --git a/lib/dpif-offload-dpdk.c b/lib/dpif-offload-dpdk.c >> index 838a6839b..2a5e67ea6 100644 >> --- a/lib/dpif-offload-dpdk.c >> +++ b/lib/dpif-offload-dpdk.c >> @@ -20,21 +20,81 @@ >> #include "dpif-offload-provider.h" >> #include "util.h" >> >> +#include "openvswitch/vlog.h" >> + >> +VLOG_DEFINE_THIS_MODULE(dpif_offload_dpdk); >> + >> +#define DEFAULT_OFFLOAD_THREAD_NB 1 >> +#define MAX_OFFLOAD_THREAD_NB 10 >> + >> +static unsigned int offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB; >> + >> +/* dpif offload interface for the dpdk rte_flow implementation. */ >> +struct dpif_offload_dpdk { >> + struct dpif_offload offload; >> + >> + /* Configuration specific variables. */ >> + struct ovsthread_once once_enable; /* Track first-time enablement. */ >> +}; >> + >> +static struct dpif_offload_dpdk * >> +dpif_offload_dpdk_cast(const struct dpif_offload *offload) >> +{ >> + dpif_offload_assert_class(offload, &dpif_offload_dpdk_class); >> + return CONTAINER_OF(offload, struct dpif_offload_dpdk, offload); >> +} >> + >> static int >> dpif_offload_dpdk_open(const struct dpif_offload_class *offload_class, >> struct dpif *dpif, struct dpif_offload **offload_) >> { >> - struct dpif_offload *offload = xmalloc(sizeof *offload); >> + struct dpif_offload_dpdk *offload; >> + >> + offload = xmalloc(sizeof *offload); >> + >> + dpif_offload_init(&offload->offload, offload_class, dpif); >> + offload->once_enable = (struct ovsthread_once) >> OVSTHREAD_ONCE_INITIALIZER; > > The 'once' structure contains a mutex. It is not designed to be > dynamically allocated, so there is no method to destroy it. But > technically we must do that in order to be POSIX-compliant. > On linux pthread_mutex doesn't hold any dynamically allocated > memory, so the destruction is not really necessary, but it can > be necessary on other platforms. You are right, I guess that’s why ASAN does not complain. Will add a destroy function and use it below. >> >> - dpif_offload_init(offload, offload_class, dpif); >> - *offload_ = offload; >> + *offload_ = &offload->offload; >> return 0; >> } >> >> static void >> -dpif_offload_dpdk_close(struct dpif_offload *dpif_offload) >> +dpif_offload_dpdk_close(struct dpif_offload *offload_) >> { >> - free(dpif_offload); >> + struct dpif_offload_dpdk *offload = dpif_offload_dpdk_cast(offload_); >> + > > Here, we technically need to destroy the once->mutex. We may need > a function for this in the thread.h. Added. >> + free(offload); >> +} >> + >> +static void >> +dpif_offload_dpdk_set_config(struct dpif_offload *offload_, >> + const struct smap *other_cfg) >> +{ >> + struct dpif_offload_dpdk *offload = dpif_offload_dpdk_cast(offload_); >> + >> + if (smap_get_bool(other_cfg, "hw-offload", false)) { >> + if (ovsthread_once_start(&offload->once_enable)) { >> + >> + offload_thread_nb = smap_get_ullong(other_cfg, >> + "n-offload-threads", >> + DEFAULT_OFFLOAD_THREAD_NB); >> + if (offload_thread_nb == 0 || >> + offload_thread_nb > MAX_OFFLOAD_THREAD_NB) { >> + VLOG_WARN("netdev: Invalid number of threads requested: %u", >> + offload_thread_nb); >> + offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB; >> + } >> + >> + if (smap_get(other_cfg, "n-offload-threads")) { >> + VLOG_INFO("Flow API using %u thread%s", >> + offload_thread_nb, >> + offload_thread_nb > 1 ? "s" : ""); >> + } >> + >> + ovsthread_once_done(&offload->once_enable); >> + } >> + } >> } >> >> struct dpif_offload_class dpif_offload_dpdk_class = { >> @@ -44,4 +104,13 @@ struct dpif_offload_class dpif_offload_dpdk_class = { >> NULL}, >> .open = dpif_offload_dpdk_open, >> .close = dpif_offload_dpdk_close, >> + .set_config = dpif_offload_dpdk_set_config, >> }; >> + >> +/* XXX: Temporary functions below, which will be removed once fully >> + * refactored. */ >> +unsigned int dpif_offload_dpdk_get_thread_nb(void); > > No need for a prototype right before the implementation. This is intentional, as these functions are temporary. Keeping the prototypes local avoids polluting headers and makes it easier to identify remaining calls during the refactor. They will be removed once the refactoring is complete. >> +unsigned int dpif_offload_dpdk_get_thread_nb(void) >> +{ >> + return offload_thread_nb; >> +} >> diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h >> index a7869b587..f0c0ea232 100644 >> --- a/lib/dpif-offload-provider.h >> +++ b/lib/dpif-offload-provider.h >> @@ -19,6 +19,8 @@ >> >> #include "dpif-provider.h" >> #include "ovs-thread.h" >> +#include "smap.h" >> +#include "util.h" >> >> #include "openvswitch/list.h" >> >> @@ -85,6 +87,11 @@ struct dpif_offload_class { >> * open() above. If your implementation accesses this provider using >> * RCU pointers, it's responsible for handling deferred deallocation. */ >> void (*close)(struct dpif_offload *); >> + /* Pass custom configuration options to the offload provider. The >> + * implementation might postpone applying the changes until run() is >> + * called. */ > > There is no run() method in this class. ACK, updated comment. >> + void (*set_config)(struct dpif_offload *, >> + const struct smap *other_config); >> }; >> >> >> @@ -94,8 +101,16 @@ extern struct dpif_offload_class dpif_offload_dpdk_class; >> extern struct dpif_offload_class dpif_offload_tc_class; >> >> >> -/* Global function, called by the dpif layer. */ >> +/* Global functions, called by the dpif layer or offload providers. */ >> void dp_offload_initialize(void); >> +void dpif_offload_set_config(struct dpif *, const struct smap *other_cfg); >> + >> +static inline void dpif_offload_assert_class( > > Name should be on a new line, since it's an implementation. Done. >> + const struct dpif_offload *dpif_offload, >> + const struct dpif_offload_class *dpif_offload_class) >> +{ >> + ovs_assert(dpif_offload->class == dpif_offload_class); >> +} >> >> >> #endif /* DPIF_OFFLOAD_PROVIDER_H */ >> diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c >> index 88efdaac8..6f504b2c2 100644 >> --- a/lib/dpif-offload-tc.c >> +++ b/lib/dpif-offload-tc.c >> @@ -18,23 +18,63 @@ >> >> #include "dpif-offload.h" >> #include "dpif-offload-provider.h" >> +#include "tc.h" >> #include "util.h" >> >> +/* dpif offload interface for the tc implementation. */ >> +struct dpif_offload_tc { >> + struct dpif_offload offload; >> + >> + /* Configuration specific variables. */ >> + struct ovsthread_once once_enable; /* Track first-time enablement. */ >> +}; >> + >> +static struct dpif_offload_tc * >> +dpif_offload_tc_cast(const struct dpif_offload *offload) >> +{ >> + dpif_offload_assert_class(offload, &dpif_offload_tc_class); >> + return CONTAINER_OF(offload, struct dpif_offload_tc, offload); >> +} >> + >> static int >> dpif_offload_tc_open(const struct dpif_offload_class *offload_class, >> struct dpif *dpif, struct dpif_offload **dpif_offload) > > This is for one of the previous patches mostly, > It seems a little strange to pass a class into class->open method. > The only use case for it is dummy and dummy_x using the same > method for both, but we can have two helper functions that call > an internal function with a specific class for those. This follows the dpif_open() pattern, which also passes the class into the ->open callback, and it’s used that way in create_dp_netdev(). If you prefer, I can refactor this to avoid passing the class and clean it up in the first three patches. >> { >> - struct dpif_offload *offload = xmalloc(sizeof *offload); >> + struct dpif_offload_tc *offload_tc; >> + >> + offload_tc = xmalloc(sizeof *offload_tc); >> + >> + dpif_offload_init(&offload_tc->offload, offload_class, dpif); >> + offload_tc->once_enable = (struct ovsthread_once) \ > > Strange line continuation. Fixed. >> + OVSTHREAD_ONCE_INITIALIZER; >> >> - dpif_offload_init(offload, offload_class, dpif); >> - *dpif_offload = offload; >> + *dpif_offload = &offload_tc->offload; >> return 0; >> } >> >> static void >> dpif_offload_tc_close(struct dpif_offload *dpif_offload) >> { >> - free(dpif_offload); >> + struct dpif_offload_tc *offload_tc = dpif_offload_tc_cast(dpif_offload); >> + >> + free(offload_tc); >> +} >> + >> +static void >> +dpif_offload_tc_set_config(struct dpif_offload *offload, >> + const struct smap *other_cfg) >> +{ >> + struct dpif_offload_tc *offload_tc = dpif_offload_tc_cast(offload); >> + >> + if (smap_get_bool(other_cfg, "hw-offload", false)) { >> + if (ovsthread_once_start(&offload_tc->once_enable)) { >> + >> + tc_set_policy(smap_get_def(other_cfg, "tc-policy", >> + TC_POLICY_DEFAULT)); >> + >> + ovsthread_once_done(&offload_tc->once_enable); >> + } >> + } >> } >> >> struct dpif_offload_class dpif_offload_tc_class = { >> @@ -44,4 +84,5 @@ struct dpif_offload_class dpif_offload_tc_class = { >> NULL}, >> .open = dpif_offload_tc_open, >> .close = dpif_offload_tc_close, >> + .set_config = dpif_offload_tc_set_config, >> }; >> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c >> index 964909e8a..901980c03 100644 >> --- a/lib/dpif-offload.c >> +++ b/lib/dpif-offload.c >> @@ -358,6 +358,23 @@ dpif_offload_detach_providers(struct dpif *dpif) >> } >> } >> >> +void >> +dpif_offload_set_config(struct dpif *dpif, const struct smap *other_cfg) >> +{ >> + struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif); >> + struct dpif_offload *offload; >> + >> + if (!dp_offload) { >> + return; >> + } >> + >> + LIST_FOR_EACH (offload, dpif_list_node, &dp_offload->offload_providers) >> { >> + if (offload->class->set_config) { >> + offload->class->set_config(offload, other_cfg); >> + } >> + } >> +} >> + >> >> void >> dpif_offload_init(struct dpif_offload *offload, >> @@ -523,6 +540,7 @@ dpif_offload_set_global_cfg(const struct smap *other_cfg) >> >> if (ovsthread_once_start(&once_enable)) { >> atomic_store_relaxed(&dpif_offload_global_enabled, true); >> + VLOG_INFO("hw-offload API Enabled"); > > Maybe "Flow HW offload is enabled"... ? Not sure, but I'd like to introduce > some distinction between the checksum and segmentation offload and the flow > offload. Ack, sounds good to me, will also update test dependencies. >> >> if (smap_get_bool(other_cfg, "offload-rebalance", false)) { >> atomic_store_relaxed(&dpif_offload_rebalance_policy, true); >> diff --git a/lib/dpif.c b/lib/dpif.c >> index 91fa00888..4d4b5127b 100644 >> --- a/lib/dpif.c >> +++ b/lib/dpif.c >> @@ -1594,6 +1594,7 @@ dpif_set_config(struct dpif *dpif, const struct smap >> *cfg) >> if (error) { >> log_operation(dpif, "set_config", error); >> } >> + dpif_offload_set_config(dpif, cfg); >> } >> >> return error; >> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c >> index 01fdadbc3..0fad1b983 100644 >> --- a/lib/netdev-offload.c >> +++ b/lib/netdev-offload.c >> @@ -61,10 +61,16 @@ VLOG_DEFINE_THIS_MODULE(netdev_offload); >> >> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); >> >> -#define DEFAULT_OFFLOAD_THREAD_NB 1 >> +/* XXX: Temporarily duplicates definition in dpif-offload-dpdk.c. */ >> #define MAX_OFFLOAD_THREAD_NB 10 >> - >> +#define DEFAULT_OFFLOAD_THREAD_NB 1 >> static unsigned int offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB; >> + >> +unsigned int dpif_offload_dpdk_get_thread_nb(void); /* XXX: Temporarily >> + * external declaration >> + * until fully >> refactored. >> + */ >> + >> DEFINE_EXTERN_PER_THREAD_DATA(netdev_offload_thread_id, OVSTHREAD_ID_UNSET); >> >> /* Protects 'netdev_flow_apis'. */ >> @@ -853,37 +859,18 @@ netdev_ports_flow_init(void) >> } >> >> void >> -netdev_set_flow_api_enabled(const struct smap *ovs_other_config) >> +netdev_set_flow_api_enabled(const struct smap *ovs_other_config OVS_UNUSED) >> { >> if (dpif_offload_is_offload_enabled()) { >> static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >> >> if (ovsthread_once_start(&once)) { >> >> - offload_thread_nb = smap_get_ullong(ovs_other_config, >> - "n-offload-threads", >> - DEFAULT_OFFLOAD_THREAD_NB); >> - if (offload_thread_nb == 0 || >> - offload_thread_nb > MAX_OFFLOAD_THREAD_NB) { >> - VLOG_WARN("netdev: Invalid number of threads requested: %u", >> - offload_thread_nb); >> - offload_thread_nb = DEFAULT_OFFLOAD_THREAD_NB; >> - } >> - >> - if (smap_get(ovs_other_config, "n-offload-threads")) { >> - VLOG_INFO("netdev: Flow API Enabled, using %u thread%s", >> - offload_thread_nb, >> - offload_thread_nb > 1 ? "s" : ""); >> - } else { >> - VLOG_INFO("netdev: Flow API Enabled"); >> - } >> - >> -#ifdef __linux__ >> - tc_set_policy(smap_get_def(ovs_other_config, "tc-policy", >> - TC_POLICY_DEFAULT)); >> +#ifdef DPDK_NETDEV >> + offload_thread_nb = dpif_offload_dpdk_get_thread_nb(); >> #endif >> - netdev_ports_flow_init(); >> >> + netdev_ports_flow_init(); >> ovsthread_once_done(&once); >> } >> } >> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at >> index ee97afae9..22f1f904a 100644 >> --- a/tests/dpif-netdev.at >> +++ b/tests/dpif-netdev.at >> @@ -414,7 +414,7 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD], >> AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >> netdev_dummy:file:dbg]) >> >> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >> - OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) >> + OVS_WAIT_UNTIL([grep "hw-offload API Enabled" ovs-vswitchd.log]) >> >> AT_CHECK([ovs-ofctl del-flows br0]) >> AT_CHECK([ovs-ofctl add-flow br0 in_port=1,actions=IN_PORT]) >> @@ -477,7 +477,7 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS], >> AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >> netdev_dummy:file:dbg]) >> >> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >> - OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) >> + OVS_WAIT_UNTIL([grep "hw-offload API Enabled" ovs-vswitchd.log]) >> >> AT_CHECK([ovs-ofctl del-flows br0]) >> >> @@ -554,7 +554,7 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS_VID_ARP], >> AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >> netdev_dummy:file:dbg]) >> >> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >> - OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) >> + OVS_WAIT_UNTIL([grep "hw-offload API Enabled" ovs-vswitchd.log]) >> >> AT_CHECK([ovs-ofctl del-flows br0]) >> >> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >> index 71cd2aed0..8894dd711 100644 >> --- a/tests/ofproto-macros.at >> +++ b/tests/ofproto-macros.at >> @@ -224,7 +224,7 @@ m4_define([_OVS_VSWITCHD_START], >> /ofproto|INFO|datapath ID changed to fedcba9876543210/d >> /dpdk|INFO|DPDK Disabled - Use other_config:dpdk-init to enable/d >> /netlink_socket|INFO|netlink: could not enable listening to all nsid/d >> -/netdev_offload|INFO|netdev: Flow API Enabled/d >> +/dpif_offload|INFO|hw-offload API Enabled/d >> /probe tc:/d >> /setting extended ack support failed/d >> /tc: Using policy/d _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
