On 1/14/26 10:18 AM, Eelco Chaudron wrote: > > > 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.
Hmm, interesting. I guess, it does that for the same resason - netdev and dummy dpif implementation handling in the same code. I guess, it's fine then to keep as-is. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
