| Message ID | 12583b97a68bfc2efd1ec5cbea591c25f926ec3a.1762950453.git.echaudro@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Architectural refactoring of hardware offload infrastructure | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
git sha: 12583b97a68bfc2efd1ec5cbea591c25f926ec3a Author: Eelco Chaudron <echaudro@redhat.com> Subject: dpif-netlink: Add API to get offloaded netdev from port_id. This patch adds an API to retrieve the netdev by port_id from either the dpif or a specific offload provider. > diff --git a/lib/dpif-offload-dpdk.c b/lib/dpif-offload-dpdk.c > index c8ae1adb2..7a4cc7292 100644 > --- a/lib/dpif-offload-dpdk.c > +++ b/lib/dpif-offload-dpdk.c > @@ -103,6 +103,20 @@ dpif_offload_dpdk_port_del(struct dpif_offload *offload_, odp_port_t port_no) > return ret; > } > > +static struct netdev * > +dpif_offload_dpdk_get_netdev(struct dpif_offload *offload_, odp_port_t port_no) This function name has a whitespace issue - the line is getting fairly long at 95 characters. Consider breaking after the return type. > +{ > + struct dpif_offload_dpdk *offload = dpif_offload_dpdk_cast(offload_); > + struct dpif_offload_port_mgr_port *port; > + > + port = dpif_offload_port_mgr_find_by_odp_port(offload->port_mgr, port_no); > + if (!port) { > + return NULL; > + } > + > + return port->netdev; Is this function missing a reference increment? The retrieved netdev might be freed while the caller is using it if no additional reference is held. > +} > + > static int > dpif_offload_dpdk_open(const struct dpif_offload_class *offload_class, > struct dpif *dpif, struct dpif_offload **offload_) [ ... ] > diff --git a/lib/dpif-offload-dummy.c b/lib/dpif-offload-dummy.c > index 4b7a292bb..96d589b9f 100644 > --- a/lib/dpif-offload-dummy.c > +++ b/lib/dpif-offload-dummy.c > @@ -97,6 +97,24 @@ dpif_offload_dummy_port_del(struct dpif_offload *dpif_offload, > return 0; > } > > +static struct netdev * > +dpif_offload_dummy_get_netdev(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_find_by_odp_port(offload_dummy->port_mgr, > + port_no); > + if (!port) { > + return NULL; > + } > + > + return port->netdev; Same issue here - is this returning a netdev without incrementing its reference count? This could lead to use-after-free if the netdev is destroyed while the caller is using it. > +} [ ... ] > diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c > index e2e18b5b1..961953961 100644 > --- a/lib/dpif-offload-tc.c > +++ b/lib/dpif-offload-tc.c > @@ -127,6 +127,22 @@ dpif_offload_tc_port_del(struct dpif_offload *dpif_offload, > return ret; > } > > +static struct netdev * > +dpif_offload_tc_get_netdev(struct dpif_offload *dpif_offload, > + odp_port_t port_no) > +{ > + struct dpif_offload_tc *offload_tc = dpif_offload_tc_cast(dpif_offload); > + struct dpif_offload_port_mgr_port *port; > + > + port = dpif_offload_port_mgr_find_by_odp_port(offload_tc->port_mgr, > + port_no); > + if (!port) { > + return NULL; > + } > + > + return port->netdev; This function also returns port->netdev directly. Should this be taking a reference on the netdev before returning it to ensure the caller can safely use it? > +} [ ... ] > diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c > index 28b3c1d21..77229c861 100644 > --- a/lib/dpif-offload.c > +++ b/lib/dpif-offload.c > @@ -1009,6 +1010,44 @@ dpif_offload_flow_dump_thread_destroy(struct dpif_flow_dump_thread *thread) > free(thread->offload_threads); > } > > +struct netdev * > +dpif_offload_offload_get_netdev_by_port_id(struct dpif_offload *offload, > + odp_port_t port_no) Function name has "offload_offload" - is this duplication intentional? > +{ > + if (!dpif_offload_is_offload_enabled() || !offload) { > + return NULL; > + } > + > + return offload->class->get_netdev(offload, port_no); The call to offload->class->get_netdev potentially returns a netdev without an incremented reference. Should callers of this function be expected to unref the returned netdev? > +} > + > +struct netdev * > +dpif_offload_get_netdev_by_port_id(struct dpif *dpif, > + struct dpif_offload **offload, > + odp_port_t port_no) > +{ > + struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif); > + struct dpif_offload *tmp_offload; > + struct netdev *netdev = NULL; > + > + if (!dp_offload || !dpif_offload_is_offload_enabled()) { > + return NULL; > + } > + > + LIST_FOR_EACH (tmp_offload, dpif_list_node, > + &dp_offload->offload_providers) { > + netdev = tmp_offload->class->get_netdev(tmp_offload, port_no); > + if (netdev) { > + if (offload) { > + *offload = tmp_offload; > + } > + break; > + } > + } > + > + return netdev; The returned netdev here comes from the same get_netdev calls that may not increment references. Are callers expected to handle this consistently? > +} [ ... ] > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 3fa96b673..d0c537bc9 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -2710,7 +2710,6 @@ ukey_netdev_unref(struct udpif_key *ukey) > static void > ukey_to_flow_netdev(struct udpif *udpif, struct udpif_key *ukey) > { > - const char *dpif_type_str = dpif_normalize_type(dpif_type(udpif->dpif)); > const struct nlattr *k; > unsigned int left; > > @@ -2722,8 +2721,8 @@ ukey_to_flow_netdev(struct udpif *udpif, struct udpif_key *ukey) > enum ovs_key_attr type = nl_attr_type(k); > > if (type == OVS_KEY_ATTR_IN_PORT) { > - ukey->in_netdev = netdev_ports_get(nl_attr_get_odp_port(k), > - dpif_type_str); > + ukey->in_netdev = dpif_offload_get_netdev_by_port_id( > + udpif->dpif, NULL, nl_attr_get_odp_port(k)); This change replaces netdev_ports_get with dpif_offload_get_netdev_by_port_id. Does netdev_ports_get increment the reference count while the new function does not? This could create a reference counting mismatch with the existing ukey_netdev_unref function. > } else if (type == OVS_KEY_ATTR_TUNNEL) { > struct flow_tnl tnl; > enum odp_key_fitness res;
diff --git a/lib/dpif-offload-dpdk.c b/lib/dpif-offload-dpdk.c index c8ae1adb2..7a4cc7292 100644 --- a/lib/dpif-offload-dpdk.c +++ b/lib/dpif-offload-dpdk.c @@ -103,6 +103,20 @@ dpif_offload_dpdk_port_del(struct dpif_offload *offload_, odp_port_t port_no) return ret; } +static struct netdev * +dpif_offload_dpdk_get_netdev(struct dpif_offload *offload_, odp_port_t port_no) +{ + struct dpif_offload_dpdk *offload = dpif_offload_dpdk_cast(offload_); + struct dpif_offload_port_mgr_port *port; + + port = dpif_offload_port_mgr_find_by_odp_port(offload->port_mgr, port_no); + if (!port) { + return NULL; + } + + return port->netdev; +} + static int dpif_offload_dpdk_open(const struct dpif_offload_class *offload_class, struct dpif *dpif, struct dpif_offload **offload_) @@ -307,6 +321,7 @@ struct dpif_offload_class dpif_offload_dpdk_class = { .port_add = dpif_offload_dpdk_port_add, .port_del = dpif_offload_dpdk_port_del, .flow_get_n_offloaded = dpif_offload_dpdk_get_n_offloaded, + .get_netdev = dpif_offload_dpdk_get_netdev, .netdev_flow_flush = dpif_offload_dpdk_netdev_flow_flush, .netdev_hw_miss_packet_postprocess = \ dpif_offload_dpdk_netdev_hw_miss_packet_postprocess, diff --git a/lib/dpif-offload-dummy.c b/lib/dpif-offload-dummy.c index 4b7a292bb..96d589b9f 100644 --- a/lib/dpif-offload-dummy.c +++ b/lib/dpif-offload-dummy.c @@ -97,6 +97,24 @@ dpif_offload_dummy_port_del(struct dpif_offload *dpif_offload, return 0; } +static struct netdev * +dpif_offload_dummy_get_netdev(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_find_by_odp_port(offload_dummy->port_mgr, + port_no); + if (!port) { + return NULL; + } + + return port->netdev; +} + static int dpif_offload_dummy_open(const struct dpif_offload_class *offload_class, struct dpif *dpif, struct dpif_offload **dpif_offload) @@ -245,6 +263,7 @@ dpif_offload_dummy_can_offload(struct dpif_offload *dpif_offload OVS_UNUSED, .can_offload = dpif_offload_dummy_can_offload, \ .port_add = dpif_offload_dummy_port_add, \ .port_del = dpif_offload_dummy_port_del, \ + .get_netdev = dpif_offload_dummy_get_netdev, \ } DEFINE_DPIF_DUMMY_CLASS(dpif_offload_dummy_class, "dummy"); diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h index d5cfa8d28..062bff9f0 100644 --- a/lib/dpif-offload-provider.h +++ b/lib/dpif-offload-provider.h @@ -224,6 +224,10 @@ struct dpif_offload_class { int (*meter_del)(const struct dpif_offload *, ofproto_meter_id meter_id, struct ofputil_meter_stats *); + /* Return the 'netdev' associated with the port_no if this offload + * provider is handling offload for this port/netdev. */ + struct netdev *(*get_netdev)(struct dpif_offload *, odp_port_t port_no); + /* These APIs operate directly on the provided netdev for performance * reasons. They are intended for use in fast path processing and should diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c index e2e18b5b1..961953961 100644 --- a/lib/dpif-offload-tc.c +++ b/lib/dpif-offload-tc.c @@ -127,6 +127,22 @@ dpif_offload_tc_port_del(struct dpif_offload *dpif_offload, return ret; } +static struct netdev * +dpif_offload_tc_get_netdev(struct dpif_offload *dpif_offload, + odp_port_t port_no) +{ + struct dpif_offload_tc *offload_tc = dpif_offload_tc_cast(dpif_offload); + struct dpif_offload_port_mgr_port *port; + + port = dpif_offload_port_mgr_find_by_odp_port(offload_tc->port_mgr, + port_no); + if (!port) { + return NULL; + } + + return port->netdev; +} + static int dpif_offload_tc_open(const struct dpif_offload_class *offload_class, struct dpif *dpif, struct dpif_offload **dpif_offload) @@ -564,5 +580,6 @@ struct dpif_offload_class dpif_offload_tc_class = { .meter_set = dpif_offload_tc_meter_set, .meter_get = dpif_offload_tc_meter_get, .meter_del = dpif_offload_tc_meter_del, + .get_netdev = dpif_offload_tc_get_netdev, .netdev_flow_flush = dpif_offload_tc_netdev_flow_flush, }; diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c index 28b3c1d21..77229c861 100644 --- a/lib/dpif-offload.c +++ b/lib/dpif-offload.c @@ -151,7 +151,8 @@ dp_offload_initialize(void) && base_dpif_offload_classes[i]->close && base_dpif_offload_classes[i]->can_offload && base_dpif_offload_classes[i]->port_add - && base_dpif_offload_classes[i]->port_del); + && base_dpif_offload_classes[i]->port_del + && base_dpif_offload_classes[i]->get_netdev); ovs_assert((base_dpif_offload_classes[i]->flow_dump_create && base_dpif_offload_classes[i]->flow_dump_next && @@ -1009,6 +1010,44 @@ dpif_offload_flow_dump_thread_destroy(struct dpif_flow_dump_thread *thread) free(thread->offload_threads); } +struct netdev * +dpif_offload_offload_get_netdev_by_port_id(struct dpif_offload *offload, + odp_port_t port_no) +{ + if (!dpif_offload_is_offload_enabled() || !offload) { + return NULL; + } + + return offload->class->get_netdev(offload, port_no); +} + +struct netdev * +dpif_offload_get_netdev_by_port_id(struct dpif *dpif, + struct dpif_offload **offload, + odp_port_t port_no) +{ + struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif); + struct dpif_offload *tmp_offload; + struct netdev *netdev = NULL; + + if (!dp_offload || !dpif_offload_is_offload_enabled()) { + return NULL; + } + + LIST_FOR_EACH (tmp_offload, dpif_list_node, + &dp_offload->offload_providers) { + netdev = tmp_offload->class->get_netdev(tmp_offload, port_no); + if (netdev) { + if (offload) { + *offload = tmp_offload; + } + break; + } + } + + return netdev; +} + int dpif_offload_netdev_flush_flows(struct netdev *netdev) diff --git a/lib/dpif-offload.h b/lib/dpif-offload.h index c7d69c3bc..31648970a 100644 --- a/lib/dpif-offload.h +++ b/lib/dpif-offload.h @@ -58,6 +58,11 @@ void dpif_offload_meter_get(const struct dpif *dpif, ofproto_meter_id meter_id, struct ofputil_meter_stats *); void dpif_offload_meter_del(const struct dpif *dpif, ofproto_meter_id meter_id, struct ofputil_meter_stats *); +struct netdev *dpif_offload_get_netdev_by_port_id(struct dpif *, + struct dpif_offload **, + odp_port_t); +struct netdev *dpif_offload_offload_get_netdev_by_port_id( + struct dpif_offload *, odp_port_t); /* Iterates through each DPIF_OFFLOAD in DPIF, using DUMP as state. * diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 3fa96b673..d0c537bc9 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -2710,7 +2710,6 @@ ukey_netdev_unref(struct udpif_key *ukey) static void ukey_to_flow_netdev(struct udpif *udpif, struct udpif_key *ukey) { - const char *dpif_type_str = dpif_normalize_type(dpif_type(udpif->dpif)); const struct nlattr *k; unsigned int left; @@ -2722,8 +2721,8 @@ ukey_to_flow_netdev(struct udpif *udpif, struct udpif_key *ukey) enum ovs_key_attr type = nl_attr_type(k); if (type == OVS_KEY_ATTR_IN_PORT) { - ukey->in_netdev = netdev_ports_get(nl_attr_get_odp_port(k), - dpif_type_str); + ukey->in_netdev = dpif_offload_get_netdev_by_port_id( + udpif->dpif, NULL, nl_attr_get_odp_port(k)); } else if (type == OVS_KEY_ATTR_TUNNEL) { struct flow_tnl tnl; enum odp_key_fitness res;
This patch adds an API to retrieve the netdev by port_id from either the dpif or a specific offload provider. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- lib/dpif-offload-dpdk.c | 15 +++++++++++++ lib/dpif-offload-dummy.c | 19 ++++++++++++++++ lib/dpif-offload-provider.h | 4 ++++ lib/dpif-offload-tc.c | 17 +++++++++++++++ lib/dpif-offload.c | 41 ++++++++++++++++++++++++++++++++++- lib/dpif-offload.h | 5 +++++ ofproto/ofproto-dpif-upcall.c | 5 ++--- 7 files changed, 102 insertions(+), 4 deletions(-)