Git SHA: 7aabe6da0d9137c7b1ae0c444ba89281932b398b
Author: Eelco Chaudron <[email protected]>
Subject: dpif-netdev: Remove indirect DPDK netdev offload API calls.
This patch removes indirect DPDK netdev offload API calls and replaces
them with direct calls. It begins the process of decoupling hardware
offload logic from dpif-netdev and transitioning it into
dpif-offload-dpdk.
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index 077bdfc09..4dd5e9789 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
[ ... ]
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 272294daa..fa265b8fc 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
[ ... ]
> @@ -416,6 +416,9 @@ void dpdk_offload_thread_set_thread_nb(unsigned int
> thread_nb);
> unsigned int dpdk_offload_thread_nb(void);
> unsigned int dpdk_offload_thread_id(void);
>
> +/* XXX: Temporarily external declarations, will be removed during cleanup. */
> +struct netdev *dpif_netdev_offload_get_netdev_by_port_id(odp_port_t);
> +
Could this function return a netdev without proper reference counting?
The original netdev_ports_get() appeared to return a netdev with an
incremented reference, but these temporary functions may not follow the
same pattern.
> @@ -2637,7 +2640,6 @@ static int
> mark_to_flow_disassociate(struct dp_netdev *dp,
> struct dp_netdev_flow *flow)
> {
> - const char *dpif_type_str = dpif_normalize_type(dp->class->type);
> struct cmap_node *mark_node = CONST_CAST(struct cmap_node *,
> &flow->mark_node);
> unsigned int tid = dpdk_offload_thread_id();
> @@ -2662,14 +2664,13 @@ mark_to_flow_disassociate(struct dp_netdev *dp,
> struct netdev *port;
> odp_port_t in_port = flow->flow.in_port.odp_port;
>
> - port = netdev_ports_get(in_port, dpif_type_str);
> + port = dpif_netdev_offload_get_netdev_by_port_id(in_port);
> if (port) {
> /* Taking a global 'port_rwlock' to fulfill thread safety
> * restrictions regarding netdev port mapping. */
> ovs_rwlock_rdlock(&dp->port_rwlock);
> - ret = netdev_flow_del(port, &flow->mega_ufid, NULL);
> + ret = netdev_offload_dpdk_flow_del(port, &flow->mega_ufid, NULL);
> ovs_rwlock_unlock(&dp->port_rwlock);
> - netdev_close(port);
> }
Does this code leak the netdev reference from
dpif_netdev_offload_get_netdev_by_port_id()? The original code called
netdev_close() but the replacement does not.
> @@ -2849,7 +2850,7 @@ dp_netdev_flow_offload_put(struct
> dp_offload_thread_item *item)
> info.flow_mark = mark;
> info.orig_in_port = offload->orig_in_port;
>
> - port = netdev_ports_get(in_port, dpif_type_str);
> + port = dpif_netdev_offload_get_netdev_by_port_id(in_port);
> if (!port) {
> goto err_free;
> }
> @@ -2857,12 +2858,10 @@ dp_netdev_flow_offload_put(struct
> dp_offload_thread_item *item)
> /* Taking a global 'port_rwlock' to fulfill thread safety
> * restrictions regarding the netdev port mapping. */
> ovs_rwlock_rdlock(&dp->port_rwlock);
> - ret = netdev_flow_put(port, &offload->match,
> - CONST_CAST(struct nlattr *, offload->actions),
> - offload->actions_len, &flow->mega_ufid, &info,
> - NULL);
> + ret = netdev_offload_dpdk_flow_put(
> + port, &offload->match, CONST_CAST(struct nlattr *, offload->actions),
> + offload->actions_len, &flow->mega_ufid, &info, NULL);
> ovs_rwlock_unlock(&dp->port_rwlock);
> - netdev_close(port);
Same issue here - does this leak the netdev reference?
> @@ -3712,8 +3710,8 @@ dpif_netdev_get_flow_offload_status(const struct
> dp_netdev *dp,
> return false;
> }
>
> - netdev = netdev_ports_get(netdev_flow->flow.in_port.odp_port,
> - dpif_normalize_type(dp->class->type));
> + netdev = dpif_netdev_offload_get_netdev_by_port_id(
> + netdev_flow->flow.in_port.odp_port);
> if (!netdev) {
> return false;
> }
[ ... ]
> - netdev_close(netdev);
> if (ret) {
> return false;
> }
And again here - is the netdev reference properly handled?
> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index 7a2ab60e7..5473dd2ea 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
[ ... ]
> +/* XXX: Temporary functions below, which will be removed once fully
> + * refactored. */
> +struct netdev *dpif_netdev_offload_get_netdev_by_port_id(odp_port_t);
> +void dpif_netdev_offload_ports_traverse(
> + bool (*cb)(struct netdev *, odp_port_t, void *), void *aux);
> +
> +struct netdev *
> +dpif_netdev_offload_get_netdev_by_port_id(odp_port_t port_no)
> +{
> + struct dp_offload *dp_offload;
> + struct dpif dpif;
> +
> + ovs_mutex_lock(&dpif_offload_mutex);
> + dp_offload = shash_find_data(&dpif_offload_providers,
> "netdev@ovs-netdev");
> + ovs_mutex_unlock(&dpif_offload_mutex);
> +
> + if (!dp_offload) {
> + return NULL;
> + }
> +
> + memset(&dpif, 0, sizeof dpif);
> + ovsrcu_set(&dpif.dp_offload, dp_offload);
> +
> + return dpif_offload_get_netdev_by_port_id(&dpif, NULL, port_no);
> +}
Does this temporary function properly handle netdev reference counting?
The caller expects to own a reference to the returned netdev, but it's
unclear if dpif_offload_get_netdev_by_port_id() provides one.
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index 7f7bd4448..cda5f3dbb 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
[ ... ]
> @@ -1160,7 +1164,7 @@ add_vport_match(struct flow_patterns *patterns,
> struct netdev *physdev;
> int ret;
>
> - physdev = netdev_ports_get(orig_in_port, tnldev->dpif_type);
> + physdev = dpif_netdev_offload_get_netdev_by_port_id(orig_in_port);
> if (physdev == NULL) {
> return -1;
> }
> @@ -1180,7 +1184,6 @@ add_vport_match(struct flow_patterns *patterns,
> add_flow_tnl_items(patterns, physdev, tnl_pmd_items, tnl_pmd_items_cnt);
>
> out:
> - netdev_close(physdev);
> return ret;
> }
Another potential netdev reference leak here.
> @@ -1843,18 +1846,17 @@ add_output_action(struct netdev *netdev,
> int ret = 0;
>
> port = nl_attr_get_odp_port(nla);
> - outdev = netdev_ports_get(port, netdev->dpif_type);
> + outdev = dpif_netdev_offload_get_netdev_by_port_id(port);
> if (outdev == NULL) {
> VLOG_DBG_RL(&rl, "Cannot find netdev for odp port %"PRIu32, port);
> return -1;
> }
> - if (!netdev_flow_api_equals(netdev, outdev) ||
> + if (!dpif_offload_netdev_same_offload(netdev, outdev) ||
> add_represented_port_action(actions, outdev)) {
> VLOG_DBG_RL(&rl, "%s: Output to port \'%s\' cannot be offloaded.",
> netdev_get_name(netdev), netdev_get_name(outdev));
> ret = -1;
> }
> - netdev_close(outdev);
> return ret;
> }
Same issue in add_output_action() - netdev reference leak?
> @@ -2180,12 +2182,11 @@ add_tnl_pop_action(struct netdev *netdev,
> int ret;
>
> port = nl_attr_get_odp_port(nla);
> - vport = netdev_ports_get(port, netdev->dpif_type);
> + vport = dpif_netdev_offload_get_netdev_by_port_id(port);
> if (vport == NULL) {
> return -1;
> }
> ret = vport_to_rte_tunnel(vport, &tunnel, netdev, &actions->s_tnl);
> - netdev_close(vport);
> if (ret) {
> return ret;
> }
And in add_tnl_pop_action() as well.
> @@ -2451,8 +2451,7 @@ netdev_offload_dpdk_flow_put(struct netdev *netdev,
> struct match *match,
> /* Extract the orig_in_port from physdev as in case of modify the one
> * provided by upper layer cannot be used.
> */
> - netdev_ports_traverse(rte_flow_data->physdev->dpif_type,
> - get_netdev_odp_cb, &aux);
> + dpif_netdev_offload_ports_traverse(get_netdev_odp_cb, &aux);
> info->orig_in_port = aux.odp_port;
> old_stats = rte_flow_data->stats;
> modification = true;
How does dpif_netdev_offload_ports_traverse() handle the netdev
references passed to the callback function? Are they properly
reference-counted or could they become invalid during traversal?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
