On 27 Jan 2026, at 23:08, Ilya Maximets wrote:
> On 1/27/26 12:02 PM, Eelco Chaudron wrote: >> Move port manager instances from provider-specific data structures >> into the common struct dpif_offload. This removes duplication across >> providers and clarifies ownership of port-related state. >> >> With the port manager owned by dpif_offload, offload providers are >> required to use the common port management APIs. This simplifies >> external port management and eliminates the need for provider-specific >> port enumeration logic. >> >> The port dump functions were also simplified using the >> CMAP_CURSOR_FOR_EACH_CONTINUE() API. >> >> Signed-off-by: Eelco Chaudron <[email protected]> >> --- [...] >> -struct dpif_offload_port_mgr_port { >> +struct dpif_offload_port { >> struct cmap_node odp_port_node; >> struct cmap_node netdev_node; >> struct cmap_node ifindex_node; >> @@ -357,30 +334,28 @@ struct dpif_offload_port_mgr_port { >> >> /* Global dpif port management library functions. */ >> struct dpif_offload_port_mgr *dpif_offload_port_mgr_init(void); >> -bool dpif_offload_port_mgr_add(struct dpif_offload_port_mgr *, >> - struct dpif_offload_port_mgr_port *, >> +void dpif_offload_port_mgr_destroy(struct dpif_offload *); >> +bool dpif_offload_port_mgr_add(struct dpif_offload *, >> + struct dpif_offload_port *, >> struct netdev *netdev, odp_port_t, >> bool need_ifindex); >> -struct dpif_offload_port_mgr_port *dpif_offload_port_mgr_remove( >> - struct dpif_offload_port_mgr *, odp_port_t); >> -void dpif_offload_port_mgr_uninit(struct dpif_offload_port_mgr *); >> -size_t dpif_offload_port_mgr_port_count(struct dpif_offload_port_mgr *); >> -struct dpif_offload_port_mgr_port *dpif_offload_port_mgr_find_by_ifindex( >> - struct dpif_offload_port_mgr *, int ifindex); >> -struct dpif_offload_port_mgr_port *dpif_offload_port_mgr_find_by_netdev( >> - struct dpif_offload_port_mgr *, struct netdev *); >> -struct dpif_offload_port_mgr_port *dpif_offload_port_mgr_find_by_odp_port( >> - struct dpif_offload_port_mgr *, odp_port_t); >> -int dpif_offload_port_mgr_port_dump_start(struct dpif_offload_port_mgr *, >> +struct dpif_offload_port *dpif_offload_port_mgr_remove(struct dpif_offload >> *, >> + odp_port_t); >> +size_t dpif_offload_port_mgr_port_count(const struct dpif_offload *); >> +struct dpif_offload_port *dpif_offload_port_mgr_find_by_ifindex( >> + const struct dpif_offload *, int ifindex); >> +struct dpif_offload_port *dpif_offload_port_mgr_find_by_netdev( >> + const struct dpif_offload *, struct netdev *); >> +struct dpif_offload_port *dpif_offload_port_mgr_find_by_odp_port( >> + const struct dpif_offload *, odp_port_t); >> +int dpif_offload_port_mgr_port_dump_start(const struct dpif_offload *, >> void **statep); >> -int dpif_offload_port_mgr_port_dump_next(struct dpif_offload_port_mgr *, >> - void *state, >> - struct dpif_offload_port *); >> -int dpif_offload_port_mgr_port_dump_done(struct dpif_offload_port_mgr *, >> - void *state); >> - >> -#define DPIF_OFFLOAD_PORT_MGR_PORT_FOR_EACH(PORT, PORT_MGR) \ >> - CMAP_FOR_EACH (PORT, odp_port_node, &(PORT_MGR)->odp_port_to_port) >> +int dpif_offload_port_mgr_port_dump_next(void *state, >> + struct dpif_offload_port_dump_port >> *); >> +int dpif_offload_port_mgr_port_dump_done(void *state); > > nit: All the port_mgr_port_dump functions can be static and not exported here. > Providers can iterate ports directly with the macro below. > > But also, do we need this API at all? AFAICT, the only user of the > dpif_offload_port_dump_start/next() is the DPIF_OFFLOAD_PORT_FOR_EACH() > macro, for which the only user is dpif_offload_netdevs_out_of_resources(), > that can actually just iterate over all the ports directly, since the > port manager is now directly accessible from the dpif-offload main module. Assuming this port walk is only needed from the dpif-offload context and not from any of the providers, I’ll optimize it for direct usage. I’ll rename the MACRO and define it in dpif-offload.c. >> + >> +#define DPIF_OFFLOAD_PORT_MGR_PORT_FOR_EACH(PORT, OFFLOAD) \ >> + CMAP_FOR_EACH (PORT, odp_port_node, &(OFFLOAD)->ports->odp_port_to_port) > > Would be great if we could rename all common functions removing the port_mgr > part from them, e.g. > dpif_offload_port_mgr_find_by_ifindex() -> > dpif_offload_port_find_by_ifindex(). > > But unfortunately, it will conflict with the top-level > dpif_offload_port_add/del() > API. So, probably should keep the naming for the functions as-is. Yes, I agreed, but the naming conflict stopped me from doing this. Will keep it as is. > However, maybe it's worth renaming the macro? i.e. > DPIF_OFFLOAD_PORT_MGR_PORT_FOR_EACH -> DPIF_OFFLOAD_PORT_FOR_EACH > in case we don't actually need the port dump API. Yes changed it to DPIF_OFFLOAD_PORT_FOR_EACH(), see earlier comment. > > WDYT? [...] >> @@ -548,7 +512,7 @@ tc_parse_flow_put(struct tc_offload *offload_tc, struct >> dpif *dpif, >> } >> >> in_port = match.flow.in_port.odp_port; >> - port = dpif_offload_port_mgr_find_by_odp_port(offload_tc->port_mgr, >> + port = dpif_offload_port_mgr_find_by_odp_port(&offload_tc->offload, >> in_port); >> if (!port) { >> return EOPNOTSUPP; >> @@ -557,12 +521,12 @@ tc_parse_flow_put(struct tc_offload *offload_tc, >> struct dpif *dpif, >> /* Check the output port for a tunnel. */ >> NL_ATTR_FOR_EACH (nla, left, put->actions, put->actions_len) { >> if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) { >> - struct dpif_offload_port_mgr_port *mgr_port; >> + struct dpif_offload_port *mgr_port; >> odp_port_t out_port; >> >> out_port = nl_attr_get_odp_port(nla); >> mgr_port = dpif_offload_port_mgr_find_by_odp_port( >> - offload_tc->port_mgr, out_port); >> + &offload_tc->offload, out_port); > > nit: While we're here, may shift the line 16 spaces to the right? Ack will fix. [...] >> bool >> dpif_offload_port_dump_next(struct dpif_offload_port_dump *dump, >> - struct dpif_offload_port *port) >> + struct dpif_offload_port_dump_port *port) >> { >> - const struct dpif_offload *offload = dump->offload; >> - >> while (true) { >> - >> if (dump->error) { >> break; >> } >> >> - dump->error = offload->class->port_dump_next(offload, dump->state, >> - port); >> + dump->error = dpif_offload_port_mgr_port_dump_next(dump->state, >> port); >> if (dump->error) { >> - offload->class->port_dump_done(offload, dump->state); >> + dpif_offload_port_mgr_port_dump_done(dump->state); >> >> if (dump->error != EOF) { >> /* If the error is not EOF, we stop dumping ports from all >> @@ -1235,9 +1235,8 @@ dpif_offload_port_dump_next(struct >> dpif_offload_port_dump *dump, >> } >> >> if (dpif_offload_get_next_offload_for_dump(dump)) { >> - offload = dump->offload; >> - dump->error = offload->class->port_dump_start(offload, >> - &dump->state); >> + dump->error = dpif_offload_port_mgr_port_dump_start( >> + dump->offload, &dump->state); > > nit: Same here. Ack will fix. [...] _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
