diff mbox series

[ovs-dev,V2,1/1] netdev-offload: Set 'miss_api_supported' to be under netdev

Message ID 20220831095955.3308650-1-elibr@nvidia.com
State Accepted
Commit 76ab364ea8facd73366411916d7d0f5ff611daed
Headers show
Series [ovs-dev,V2,1/1] netdev-offload: Set 'miss_api_supported' to be under netdev | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Eli Britstein Aug. 31, 2022, 9:59 a.m. UTC 
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 <elibr@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

Comments

Ilya Maximets Oct. 26, 2022, 11:01 a.m. UTC | #1 
On 8/31/22 11:59, Eli Britstein wrote:
> 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 <elibr@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(-)

Thanks!  Applied and backported down to 2.17.

Best regards, Ilya Maximets.
diff mbox series

Patch

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. */