diff mbox series

[ovs-dev,1/2] netdev-offload: Expose error when init_flow_api() returns EBUSY.

Message ID 20220808165401.3684424-1-hzhou@ovn.org
State Changes Requested
Headers show
Series [ovs-dev,1/2] netdev-offload: Expose error when init_flow_api() returns EBUSY. | expand

Checks

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

Commit Message

Han Zhou Aug. 8, 2022, 4:54 p.m. UTC 
init_flow_api() can fail due to errors, such as netlink "Device or
resource busy". Today the errors are ignored except just an INFO log.
This ends up with an interface created without offload enabled, and
application won't notice it. This patch expose the error to the OVSDB
record, and reverts the dpif port changes upon failure.

Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 lib/dpif.c           | 12 ++++++++++--
 lib/netdev-offload.c | 32 +++++++++++++++++++++++---------
 lib/netdev-offload.h |  1 -
 3 files changed, 33 insertions(+), 12 deletions(-)

Comments

Mike Pattrick Oct. 7, 2022, 6:28 p.m. UTC | #1 
On Mon, Aug 8, 2022 at 12:54 PM Han Zhou <hzhou@ovn.org> wrote:
>
> init_flow_api() can fail due to errors, such as netlink "Device or
> resource busy". Today the errors are ignored except just an INFO log.
> This ends up with an interface created without offload enabled, and
> application won't notice it. This patch expose the error to the OVSDB
> record, and reverts the dpif port changes upon failure.
>
> Signed-off-by: Han Zhou <hzhou@ovn.org>

This seems reasonable to me.

Acked-by: Mike Pattrick <mkp@redhat.com>
diff mbox series

Patch

diff --git a/lib/dpif.c b/lib/dpif.c
index 40f5fe446..b68929bae 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -609,9 +609,17 @@  dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop)
             dpif_port.type = CONST_CAST(char *, netdev_get_type(netdev));
             dpif_port.name = CONST_CAST(char *, netdev_name);
             dpif_port.port_no = port_no;
-            netdev_ports_insert(netdev, &dpif_port);
+            error = netdev_ports_insert(netdev, &dpif_port);
+            if (error) {
+                if (error == EEXIST) {
+                    error = 0;
+                } else {
+                    dpif->dpif_class->port_del(dpif, port_no);
+                }
+            }
         }
-    } else {
+    }
+    if (error) {
         VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s",
                      dpif_name(dpif), netdev_name, ovs_strerror(error));
         port_no = ODPP_NONE;
diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
index 9fde5f7a9..f30b20673 100644
--- a/lib/netdev-offload.c
+++ b/lib/netdev-offload.c
@@ -179,21 +179,29 @@  static int
 netdev_assign_flow_api(struct netdev *netdev)
 {
     struct netdev_registered_flow_api *rfa;
-
+    int ret = -1;
     CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
-        if (!rfa->flow_api->init_flow_api(netdev)) {
+        int error = rfa->flow_api->init_flow_api(netdev);
+        if (!error) {
             ovs_refcount_ref(&rfa->refcnt);
             ovsrcu_set(&netdev->flow_api, rfa->flow_api);
             VLOG_INFO("%s: Assigned flow API '%s'.",
                       netdev_get_name(netdev), rfa->flow_api->type);
             return 0;
+        } else {
+            VLOG_DBG("%s: flow API '%s' is not suitable.",
+                     netdev_get_name(netdev), rfa->flow_api->type);
+            if (ret == EBUSY) {
+                /* If all API fail and at least one of the error is EBUSY, we
+                 * will return this error code EBUSY so that the upper layer
+                 * can decide to retry later. */
+                ret = error;
+            }
         }
-        VLOG_DBG("%s: flow API '%s' is not suitable.",
-                 netdev_get_name(netdev), rfa->flow_api->type);
     }
     VLOG_INFO("%s: No suitable flow API found.", netdev_get_name(netdev));
 
-    return -1;
+    return ret;
 }
 
 void
@@ -368,7 +376,7 @@  netdev_flow_get_n_flows(struct netdev *netdev, uint64_t *n_flows)
            : EOPNOTSUPP;
 }
 
-int
+static int
 netdev_init_flow_api(struct netdev *netdev)
 {
     if (!netdev_is_flow_api_enabled()) {
@@ -379,7 +387,10 @@  netdev_init_flow_api(struct netdev *netdev)
         return 0;
     }
 
-    if (netdev_assign_flow_api(netdev)) {
+    int error = netdev_assign_flow_api(netdev);
+    if (error == EBUSY) {
+        return error;
+    } else if (error) {
         return EOPNOTSUPP;
     }
 
@@ -729,8 +740,11 @@  netdev_ports_insert(struct netdev *netdev, struct dpif_port *dpif_port)
                 netdev_ports_hash(dpif_port->port_no, dpif_type));
     ovs_rwlock_unlock(&netdev_hmap_rwlock);
 
-    netdev_init_flow_api(netdev);
-
+    int error = netdev_init_flow_api(netdev);
+    if (error && error != EOPNOTSUPP) {
+        netdev_ports_remove(dpif_port->port_no, dpif_type);
+        return error;
+    }
     return 0;
 }
 
diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
index 249a3102a..2974e5497 100644
--- a/lib/netdev-offload.h
+++ b/lib/netdev-offload.h
@@ -118,7 +118,6 @@  int netdev_flow_get(struct netdev *, struct match *, struct nlattr **actions,
                     struct dpif_flow_attrs *, struct ofpbuf *wbuffer);
 int netdev_flow_del(struct netdev *, const ovs_u128 *,
                     struct dpif_flow_stats *);
-int netdev_init_flow_api(struct netdev *);
 void netdev_uninit_flow_api(struct netdev *);
 uint32_t netdev_get_block_id(struct netdev *);
 int netdev_get_hw_info(struct netdev *, int);