On 13 Jan 2026, at 21:03, Ilya Maximets wrote:
> On 1/12/26 12:20 PM, Eelco Chaudron wrote: >> This patch introduces a common port management layer for >> offload providers and integrates it into the dpif-offload >> subsystem. Existing dummy offload provider is updated to >> use the new APIs. >> >> 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. All comments are taken care of in v6. See explanation below on the port_mgr APIS. //Eelco >> --- >> >> v2 changes: >> - Fixed indentation issues. >> - Added comment to dpif_offload_attach_providers(). >> - Fix netdev reference issue in dpif_offload_port_mgr_add(). >> >> v3 changes: >> - Fixed indentation issues. >> - Removed unused arguments. >> - Fixed { alignment. >> - Removed unnecessary ?: constructs. >> - Free port netdev in rcu callback. >> >> v4 changes: >> - Removed leftover netdev_close(port->netdev) causing >> netdev reference count issues. >> >> v5 changes: >> - Moved DPIF_OFFLOAD_PORT_MGR_PORT_FOR_EACH() to this patch >> from patch 18. >> --- >> lib/dpif-offload-dpdk.c | 17 +++ >> lib/dpif-offload-dummy.c | 155 ++++++++++++++++++++- >> lib/dpif-offload-provider.h | 73 ++++++++++ >> lib/dpif-offload-tc.c | 20 +++ >> lib/dpif-offload.c | 265 +++++++++++++++++++++++++++++++++++- >> lib/dpif.c | 11 +- >> lib/dummy.h | 3 + >> lib/netdev-dpdk.c | 4 +- >> lib/netdev-dpdk.h | 2 +- >> lib/netdev-dummy.c | 20 +-- >> lib/netdev-offload-dpdk.c | 4 +- >> lib/netdev-provider.h | 1 + >> 12 files changed, 552 insertions(+), 23 deletions(-) >> >> diff --git a/lib/dpif-offload-dpdk.c b/lib/dpif-offload-dpdk.c >> index 2a5e67ea6..e6e467629 100644 >> --- a/lib/dpif-offload-dpdk.c >> +++ b/lib/dpif-offload-dpdk.c >> @@ -18,6 +18,8 @@ >> >> #include "dpif-offload.h" >> #include "dpif-offload-provider.h" >> +#include "netdev-provider.h" >> +#include "netdev-vport.h" >> #include "util.h" >> >> #include "openvswitch/vlog.h" >> @@ -97,6 +99,20 @@ dpif_offload_dpdk_set_config(struct dpif_offload >> *offload_, >> } >> } >> >> +static bool >> +dpif_offload_dpdk_can_offload(struct dpif_offload *offload OVS_UNUSED, >> + struct netdev *netdev) >> +{ >> + if (netdev_vport_is_vport_class(netdev->netdev_class) >> + && strcmp(netdev_get_dpif_type(netdev), "netdev")) { >> + VLOG_DBG("%s: vport doesn't belong to the netdev datapath, >> skipping", >> + netdev_get_name(netdev)); >> + return false; >> + } >> + >> + return netdev_dpdk_flow_api_supported(netdev, true); >> +} >> + >> struct dpif_offload_class dpif_offload_dpdk_class = { >> .type = "dpdk", >> .supported_dpif_types = (const char *const[]) { >> @@ -105,6 +121,7 @@ struct dpif_offload_class dpif_offload_dpdk_class = { >> .open = dpif_offload_dpdk_open, >> .close = dpif_offload_dpdk_close, >> .set_config = dpif_offload_dpdk_set_config, >> + .can_offload = dpif_offload_dpdk_can_offload, >> }; >> >> /* XXX: Temporary functions below, which will be removed once fully >> diff --git a/lib/dpif-offload-dummy.c b/lib/dpif-offload-dummy.c >> index 4ff63bdb6..795c82a6c 100644 >> --- a/lib/dpif-offload-dummy.c >> +++ b/lib/dpif-offload-dummy.c >> @@ -15,27 +15,170 @@ >> */ >> >> #include <config.h> >> +#include <errno.h> >> >> #include "dpif.h" >> #include "dpif-offload.h" >> #include "dpif-offload-provider.h" >> +#include "dummy.h" >> +#include "netdev-provider.h" >> #include "util.h" >> >> +struct dpif_offload_dummy { >> + struct dpif_offload offload; >> + struct dpif_offload_port_mgr *port_mgr; >> + >> + /* Configuration specific variables. */ >> + struct ovsthread_once once_enable; /* Track first-time enablement. */ >> +}; >> + >> +static struct dpif_offload_dummy * >> +dpif_offload_dummy_cast(const struct dpif_offload *offload) >> +{ >> + return CONTAINER_OF(offload, struct dpif_offload_dummy, offload); >> +} >> + >> +static void >> +dpif_offload_dummy_enable_offload(struct dpif_offload *dpif_offload, >> + struct dpif_offload_port_mgr_port *port) >> +{ >> + dpif_offload_set_netdev_offload(port->netdev, dpif_offload); >> +} >> + >> +static void >> +dpif_offload_dummy_cleanup_offload(struct dpif_offload_port_mgr_port *port) >> +{ >> + dpif_offload_set_netdev_offload(port->netdev, NULL); >> +} >> + >> +static int >> +dpif_offload_dummy_port_add(struct dpif_offload *dpif_offload, >> + struct netdev *netdev, odp_port_t port_no) >> +{ >> + struct dpif_offload_port_mgr_port *port = xmalloc(sizeof *port); >> + struct dpif_offload_dummy *offload_dummy; >> + >> + offload_dummy = dpif_offload_dummy_cast(dpif_offload); >> + if (dpif_offload_port_mgr_add(offload_dummy->port_mgr, port, netdev, >> + port_no, false)) { >> + >> + if (dpif_offload_is_offload_enabled()) { >> + dpif_offload_dummy_enable_offload(dpif_offload, port); >> + } >> + return 0; >> + } >> + >> + free(port); >> + return EEXIST; >> +} >> + >> +static void >> +dpif_offload_dummy_free_port(struct dpif_offload_port_mgr_port *port) >> +{ >> + netdev_close(port->netdev); >> + free(port); >> +} >> + >> +static int >> +dpif_offload_dummy_port_del(struct dpif_offload *dpif_offload, >> + odp_port_t port_no) >> +{ >> + struct dpif_offload_dummy *offload_dummy; >> + struct dpif_offload_port_mgr_port *port; >> + >> + offload_dummy = dpif_offload_dummy_cast(dpif_offload); >> + >> + port = dpif_offload_port_mgr_remove(offload_dummy->port_mgr, port_no, >> + true); >> + if (port) { >> + if (dpif_offload_is_offload_enabled()) { >> + dpif_offload_dummy_cleanup_offload(port); >> + } >> + ovsrcu_postpone(dpif_offload_dummy_free_port, port); >> + } >> + return 0; >> +} >> + >> static int >> dpif_offload_dummy_open(const struct dpif_offload_class *offload_class, >> struct dpif *dpif, struct dpif_offload >> **dpif_offload) >> { >> - struct dpif_offload *offload = xmalloc(sizeof *offload); >> + struct dpif_offload_dummy *offload_dummy; >> >> - dpif_offload_init(offload, offload_class, dpif); >> - *dpif_offload = offload; >> + offload_dummy = xmalloc(sizeof *offload_dummy); >> + >> + dpif_offload_init(&offload_dummy->offload, offload_class, dpif); >> + offload_dummy->port_mgr = dpif_offload_port_mgr_init(); > > Is there a reason why port manager is given to the provider to maintain > and not part of the generic dpif_offload structure? The only reason for > this seems to be that we could have a provider that doesn't use this > implementation of the port manager, but uses something else. Are we > anticipating such a provider? All of the existing ones use the port manager. > > Asking because it's a little awkward that provider owns the port manager, > but doesn't know how it works, and the generic dpif_offload code knows how > the port manager works but doesn't own one. So, when provider needs a port, > it asks the generic code to access the port manager, and when the generic > code wants to access ports it asks providers to call back. We could probbaly > drop the dpif_offload_port_dump_* family of functions we could just > iterateover > the cmap without talking to the provider. We'd just need an accessor for the > provider to get the port manager. In this case we would also not need the > separate dpif_offload_port structure and could rename the port manager's > dpif_offload_port_mgr_port structure into dpif_offload_port instead. > > It's more of a question and we can keep the logic as-is for now and maybe > refine it later, but I just want to understand. As we briefly discussed online, I’ll keep the current implementation for now, but will follow it up with a patch to integrate this into dpif_offload and simplify the callback APIs. > But also, the abstraction seems a bit broken as providers are using both the > dpif_offload_port_mgr_traverse_ports() and > DPIF_OFFLOAD_PORT_MGR_PORT_FOR_EACH, > which kind of requires knowledge of the structure of the port manager. So, > I'm > not sure why we have both the iterator and the traversal function, as the > traversal function is just a wrapper around the iterator. I will remove the dpif_offload_port_mgr_traverse_ports() API in the v6 patch. >> + offload_dummy->once_enable = (struct ovsthread_once) >> + OVSTHREAD_ONCE_INITIALIZER; > > nit: Might be better to move the cast to the next line as well. > Same for other providers. Fixed in v6. >> + >> + *dpif_offload = &offload_dummy->offload; >> return 0; >> } >> >> +static bool >> +dpif_offload_dummy_cleanup_port(struct dpif_offload_port_mgr_port *port, >> + void *aux) >> +{ >> + struct dpif_offload *offload = aux; >> + >> + dpif_offload_dummy_port_del(offload, port->port_no); >> + return false; >> +} >> + >> static void >> dpif_offload_dummy_close(struct dpif_offload *dpif_offload) >> { >> - free(dpif_offload); >> + struct dpif_offload_dummy *offload_dummy; >> + >> + offload_dummy = dpif_offload_dummy_cast(dpif_offload); >> + >> + /* The ofproto layer may not call dpif_port_del() for all ports, >> + * especially internal ones, so we need to clean up any remaining >> ports. */ >> + dpif_offload_port_mgr_traverse_ports(offload_dummy->port_mgr, >> + dpif_offload_dummy_cleanup_port, >> + dpif_offload); >> + >> + dpif_offload_port_mgr_uninit(offload_dummy->port_mgr); >> + free(offload_dummy); >> +} >> + >> +static bool >> +dpif_offload_dummy_late_enable(struct dpif_offload_port_mgr_port *port, >> + void *aux) >> +{ >> + dpif_offload_dummy_enable_offload(aux, port); >> + return false; >> +} >> + >> +static void >> +dpif_offload_dummy_set_config(struct dpif_offload *dpif_offload, >> + const struct smap *other_cfg) >> +{ >> + struct dpif_offload_dummy *offload_dummy; >> + >> + offload_dummy = dpif_offload_dummy_cast(dpif_offload); >> + >> + /* 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. */ > > You removed this comment form the other providers after the Aaron's > review. Should we remove this one as well? Ack will do. >> + if (smap_get_bool(other_cfg, "hw-offload", false)) { >> + if (ovsthread_once_start(&offload_dummy->once_enable)) { >> + >> + dpif_offload_port_mgr_traverse_ports( >> + offload_dummy->port_mgr, dpif_offload_dummy_late_enable, >> + dpif_offload); >> + >> + ovsthread_once_done(&offload_dummy->once_enable); >> + } >> + } >> +} >> + >> +static bool >> +dpif_offload_dummy_can_offload(struct dpif_offload *dpif_offload OVS_UNUSED, >> + struct netdev *netdev) >> +{ >> + return is_dummy_netdev_class(netdev->netdev_class); >> } >> >> #define DEFINE_DPIF_DUMMY_CLASS(NAME, TYPE_STR) \ >> @@ -46,6 +189,10 @@ dpif_offload_dummy_close(struct dpif_offload >> *dpif_offload) >> NULL}, \ >> .open = dpif_offload_dummy_open, \ >> .close = dpif_offload_dummy_close, \ >> + .set_config = dpif_offload_dummy_set_config, \ >> + .can_offload = dpif_offload_dummy_can_offload, \ >> + .port_add = dpif_offload_dummy_port_add, \ >> + .port_del = dpif_offload_dummy_port_del, \ >> } >> >> DEFINE_DPIF_DUMMY_CLASS(dpif_offload_dummy_class, "dummy"); >> diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h >> index f0c0ea232..d53cc9f18 100644 >> --- a/lib/dpif-offload-provider.h >> +++ b/lib/dpif-offload-provider.h >> @@ -17,6 +17,7 @@ >> #ifndef DPIF_OFFLOAD_PROVIDER_H >> #define DPIF_OFFLOAD_PROVIDER_H >> >> +#include "cmap.h" >> #include "dpif-provider.h" >> #include "ovs-thread.h" >> #include "smap.h" >> @@ -92,6 +93,31 @@ struct dpif_offload_class { >> * called. */ >> void (*set_config)(struct dpif_offload *, >> const struct smap *other_config); >> + >> + /* Verifies whether the offload provider supports offloading flows for >> the >> + * given 'netdev'. Returns 'false' if the provider lacks the >> capabilities >> + * to offload on this port, otherwise returns 'true'. */ >> + bool (*can_offload)(struct dpif_offload *, >> + struct netdev *); >> + >> + /* This callback is invoked when a 'netdev' port has been successfully >> + * added to the dpif and should be handled by this offload provider. >> + * It is assumed that the `can_offload` callback was previously called > > nit: Other comments use normal single quotes. Fixed in v6. >> + * and returned 'true' before this function is executed. */ >> + int (*port_add)(struct dpif_offload *, struct netdev *, >> + odp_port_t port_no); >> + >> + /* This callback is invoked when the 'port_no' port has been >> successfully >> + * removed from the dpif. Note that it is called for every deleted >> port, >> + * even if 'port_added' was never called, as the framework does not >> track >> + * added ports. */ >> + int (*port_del)(struct dpif_offload *, odp_port_t port_no); >> + >> + /* Refreshes the configuration of 'port_no' port. The implementation >> might >> + * postpone applying the changes until run() is called. The same note > > There is no run() in this class. Fixed in v6. [...] _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
