Commit [1] introduced a flag in dpif-netdev level, to optimize performance and avoid hw_miss_packet_recover() for devices with no such support. However, there is a race condition between traffic processing and assigning a 'flow_api' object to the netdev. In such case, EOPNOTSUPP is returned by netdev_hw_miss_packet_recover() in netdev-offload.c layer because 'flow_api' is not yet initialized. As a result, the flag is falsely disabled, and subsequent packets won't be recovered, though they should.
In order to fix it, move the flag to be in netdev-offload layer, to avoid that race. [1]: 6e50c1651869 ("dpif-netdev: Avoid hw_miss_packet_recover() for devices with no support.") Fixes: 6e50c1651869 ("dpif-netdev: Avoid hw_miss_packet_recover() for devices with no support.") Signed-off-by: Eli Britstein <el...@nvidia.com> --- lib/dpif-netdev.c | 18 +++++++----------- lib/netdev-offload.c | 28 +++++++++++++++++++++++----- lib/netdev-offload.h | 2 ++ lib/netdev.c | 1 + 4 files changed, 33 insertions(+), 16 deletions(-) Revision history: - v2: bool -> atomic_bool diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index a45b460145..2c08a71c8d 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -431,7 +431,6 @@ struct dp_netdev_rxq { unsigned intrvl_idx; /* Write index for 'cycles_intrvl'. */ struct dp_netdev_pmd_thread *pmd; /* pmd thread that polls this queue. */ bool is_vhost; /* Is rxq of a vhost port. */ - bool hw_miss_api_supported; /* hw_miss_packet_recover() supported.*/ /* Counters of cycles spent successfully polling and processing pkts. */ atomic_ullong cycles[RXQ_N_CYCLES]; @@ -5416,7 +5415,6 @@ port_reconfigure(struct dp_netdev_port *port) port->rxqs[i].port = port; port->rxqs[i].is_vhost = !strncmp(port->type, "dpdkvhost", 9); - port->rxqs[i].hw_miss_api_supported = true; err = netdev_rxq_open(netdev, &port->rxqs[i].rx, i); if (err) { @@ -8034,17 +8032,15 @@ dp_netdev_hw_flow(const struct dp_netdev_pmd_thread *pmd, #ifdef ALLOW_EXPERIMENTAL_API /* Packet restoration API required. */ /* Restore the packet if HW processing was terminated before completion. */ struct dp_netdev_rxq *rxq = pmd->ctx.last_rxq; + bool miss_api_supported; - if (rxq->hw_miss_api_supported) { + atomic_read_relaxed(&rxq->port->netdev->hw_info.miss_api_supported, + &miss_api_supported); + if (miss_api_supported) { int err = netdev_hw_miss_packet_recover(rxq->port->netdev, packet); - if (err) { - if (err != EOPNOTSUPP) { - COVERAGE_INC(datapath_drop_hw_miss_recover); - return -1; - } else { - /* API unsupported by the port; avoid subsequent calls. */ - rxq->hw_miss_api_supported = false; - } + if (err && err != EOPNOTSUPP) { + COVERAGE_INC(datapath_drop_hw_miss_recover); + return -1; } } #endif diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index 9fde5f7a95..4592262bd3 100644 --- a/lib/netdev-offload.c +++ b/lib/netdev-offload.c @@ -183,6 +183,7 @@ netdev_assign_flow_api(struct netdev *netdev) CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) { if (!rfa->flow_api->init_flow_api(netdev)) { ovs_refcount_ref(&rfa->refcnt); + atomic_store_relaxed(&netdev->hw_info.miss_api_supported, true); ovsrcu_set(&netdev->flow_api, rfa->flow_api); VLOG_INFO("%s: Assigned flow API '%s'.", netdev_get_name(netdev), rfa->flow_api->type); @@ -191,6 +192,7 @@ netdev_assign_flow_api(struct netdev *netdev) VLOG_DBG("%s: flow API '%s' is not suitable.", netdev_get_name(netdev), rfa->flow_api->type); } + atomic_store_relaxed(&netdev->hw_info.miss_api_supported, false); VLOG_INFO("%s: No suitable flow API found.", netdev_get_name(netdev)); return -1; @@ -322,12 +324,28 @@ int netdev_hw_miss_packet_recover(struct netdev *netdev, struct dp_packet *packet) { - const struct netdev_flow_api *flow_api = - ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); + const struct netdev_flow_api *flow_api; + bool miss_api_supported; + int rv; + + atomic_read_relaxed(&netdev->hw_info.miss_api_supported, + &miss_api_supported); + if (!miss_api_supported) { + return EOPNOTSUPP; + } + + flow_api = ovsrcu_get(const struct netdev_flow_api *, &netdev->flow_api); + if (!flow_api || !flow_api->hw_miss_packet_recover) { + return EOPNOTSUPP; + } + + rv = flow_api->hw_miss_packet_recover(netdev, packet); + if (rv == EOPNOTSUPP) { + /* API unsupported by the port; avoid subsequent calls. */ + atomic_store_relaxed(&netdev->hw_info.miss_api_supported, false); + } - return (flow_api && flow_api->hw_miss_packet_recover) - ? flow_api->hw_miss_packet_recover(netdev, packet) - : EOPNOTSUPP; + return rv; } int diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h index 180d3f95f0..edc843cd99 100644 --- a/lib/netdev-offload.h +++ b/lib/netdev-offload.h @@ -20,6 +20,7 @@ #include "openvswitch/netdev.h" #include "openvswitch/types.h" +#include "ovs-atomic.h" #include "ovs-rcu.h" #include "ovs-thread.h" #include "openvswitch/ofp-meter.h" @@ -46,6 +47,7 @@ struct ovs_action_push_tnl; /* Offload-capable (HW) netdev information */ struct netdev_hw_info { bool oor; /* Out of Offload Resources ? */ + atomic_bool miss_api_supported; /* hw_miss_packet_recover() supported.*/ int offload_count; /* Pending (non-offloaded) flow count */ int pending_count; /* Offloaded flow count */ OVSRCU_TYPE(void *) offload_data; /* Offload metadata. */ diff --git a/lib/netdev.c b/lib/netdev.c index ce0d4117ac..c797783782 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -431,6 +431,7 @@ netdev_open(const char *name, const char *type, struct netdev **netdevp) seq_read(netdev->reconfigure_seq); ovsrcu_set(&netdev->flow_api, NULL); netdev->hw_info.oor = false; + atomic_init(&netdev->hw_info.miss_api_supported, false); netdev->node = shash_add(&netdev_shash, name, netdev); /* By default enable one tx and rx queue per netdev. */ -- 2.26.2.1730.g385c171 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev