diff mbox series

[ovs-dev,2/2] dpif-netdev: Fix flushing of a vport

Message ID 20220905144603.3585105-2-elibr@nvidia.com
State Changes Requested
Headers show
Series [ovs-dev,1/2] netdev-offload-dpdk: Fix flushing of a physdev | 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

Eli Britstein Sept. 5, 2022, 2:46 p.m. UTC 
When using a userspace vport ("vxlan0"), dpif-netdev adds an additional
netdev ("vxlan_sys_4789"). The dpif netdev ("vxlan0") is added to the
netdev-offload ports map, thus flows are associated on this netdev.

However, flushing is done on the dpif-netdev level ("vxlan_sys_4789"),
and relevant offload flows are not destroyed.

To fix it, add the datapath netdev to the netdev-offload ports map. In
case there is no different internal netdev, use the dpif netdev, as before.

Fixes: adbd4301a249 ("netdev-offload-dpdk: Use per-netdev offload metadata.")
Signed-off-by: Eli Britstein <elibr@nvidia.com>
---
 lib/dpif-netdev.c   | 15 ++++++++++-----
 lib/dpif-netlink.c  |  5 ++++-
 lib/dpif-provider.h |  5 +++--
 lib/dpif.c          |  8 +++++---
 4 files changed, 22 insertions(+), 11 deletions(-)

Comments

David Marchand June 9, 2023, 12:32 p.m. UTC | #1 
On Mon, Sep 5, 2022 at 4:47 PM Eli Britstein via dev
<ovs-dev@openvswitch.org> wrote:
>
> When using a userspace vport ("vxlan0"), dpif-netdev adds an additional
> netdev ("vxlan_sys_4789"). The dpif netdev ("vxlan0") is added to the
> netdev-offload ports map, thus flows are associated on this netdev.
>
> However, flushing is done on the dpif-netdev level ("vxlan_sys_4789"),
> and relevant offload flows are not destroyed.
>
> To fix it, add the datapath netdev to the netdev-offload ports map. In
> case there is no different internal netdev, use the dpif netdev, as before.
>
> Fixes: adbd4301a249 ("netdev-offload-dpdk: Use per-netdev offload metadata.")
> Signed-off-by: Eli Britstein <elibr@nvidia.com>

Sorry, this mail was in my drafts list as I was looking at patch 1 of
the series, and I forgot about it... sending now.


I was wondering if this specificity could be hidden within dpif-netdev.c.
But I found no elegant solution.
From my limited understanding, the dpif generic layer populates the
offload map for use by the dpif implementation later.
So it needs to be aware of the dpif implementation netdevs.

I ran a few simple scenario deleting vxlan ports, flushing flows
manually and I saw nothing broken.

Reviewed-by: David Marchand <david.marchand@redhat.com>
diff mbox series

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index a45b46014..b251de881 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -536,7 +536,8 @@  static int get_port_by_name(struct dp_netdev *dp, const char *devname,
 static void dp_netdev_free(struct dp_netdev *)
     OVS_REQUIRES(dp_netdev_mutex);
 static int do_add_port(struct dp_netdev *dp, const char *devname,
-                       const char *type, odp_port_t port_no)
+                       const char *type, odp_port_t port_no,
+                       struct netdev **datapath_netdev)
     OVS_REQ_WRLOCK(dp->port_rwlock);
 static void do_del_port(struct dp_netdev *dp, struct dp_netdev_port *)
     OVS_REQ_WRLOCK(dp->port_rwlock);
@@ -1845,7 +1846,7 @@  create_dp_netdev(const char *name, const struct dpif_class *class,
 
     error = do_add_port(dp, name, dpif_netdev_port_open_type(dp->class,
                                                              "internal"),
-                        ODPP_LOCAL);
+                        ODPP_LOCAL, NULL);
     ovs_rwlock_unlock(&dp->port_rwlock);
     if (error) {
         dp_netdev_free(dp);
@@ -2112,7 +2113,7 @@  out:
 
 static int
 do_add_port(struct dp_netdev *dp, const char *devname, const char *type,
-            odp_port_t port_no)
+            odp_port_t port_no, struct netdev **datapath_netdev)
     OVS_REQ_WRLOCK(dp->port_rwlock)
 {
     struct netdev_saved_flags *sf;
@@ -2128,6 +2129,9 @@  do_add_port(struct dp_netdev *dp, const char *devname, const char *type,
     if (error) {
         return error;
     }
+    if (datapath_netdev) {
+        *datapath_netdev = port->netdev;
+    }
 
     hmap_insert(&dp->ports, &port->node, hash_port_no(port_no));
     seq_change(dp->port_seq);
@@ -2157,7 +2161,7 @@  do_add_port(struct dp_netdev *dp, const char *devname, const char *type,
 
 static int
 dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev,
-                     odp_port_t *port_nop)
+                     odp_port_t *port_nop, struct netdev **datapath_netdev)
 {
     struct dp_netdev *dp = get_dp_netdev(dpif);
     char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
@@ -2176,7 +2180,8 @@  dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev,
     }
     if (!error) {
         *port_nop = port_no;
-        error = do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no);
+        error = do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no,
+                            datapath_netdev);
     }
     ovs_rwlock_unlock(&dp->port_rwlock);
 
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index a620a6ec5..55d5a4593 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -1139,7 +1139,7 @@  dpif_netlink_rtnl_port_create_and_add(struct dpif_netlink *dpif,
 
 static int
 dpif_netlink_port_add(struct dpif *dpif_, struct netdev *netdev,
-                      odp_port_t *port_nop)
+                      odp_port_t *port_nop, struct netdev **datapath_netdev)
 {
     struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
     int error = EOPNOTSUPP;
@@ -1152,6 +1152,9 @@  dpif_netlink_port_add(struct dpif *dpif_, struct netdev *netdev,
         error = dpif_netlink_port_add_compat(dpif, netdev, port_nop);
     }
     fat_rwlock_unlock(&dpif->upcall_lock);
+    if (datapath_netdev) {
+        *datapath_netdev = netdev;
+    }
 
     return error;
 }
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 12477a24f..b79b10e6c 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -195,10 +195,11 @@  struct dpif_class {
      * ODPP_NONE, attempts to use that as the port's port number.
      *
      * If port is successfully added, sets '*port_no' to the new port's
-     * port number.  Returns EBUSY if caller attempted to choose a port
+     * port number, and datapath_netdev to a potentially created netdev in the
+     * dpif-class level.  Returns EBUSY if caller attempted to choose a port
      * number, and it was in use. */
     int (*port_add)(struct dpif *dpif, struct netdev *netdev,
-                    odp_port_t *port_no);
+                    odp_port_t *port_no, struct netdev **datapath_netdev);
 
     /* Removes port numbered 'port_no' from 'dpif'. */
     int (*port_del)(struct dpif *dpif, odp_port_t port_no);
diff --git a/lib/dpif.c b/lib/dpif.c
index 40f5fe446..2a1679ce5 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -586,6 +586,7 @@  dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop)
 {
     const char *netdev_name = netdev_get_name(netdev);
     odp_port_t port_no = ODPP_NONE;
+    struct netdev *datapath_netdev;
     int error;
 
     COVERAGE_INC(dpif_port_add);
@@ -594,7 +595,8 @@  dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop)
         port_no = *port_nop;
     }
 
-    error = dpif->dpif_class->port_add(dpif, netdev, &port_no);
+    error = dpif->dpif_class->port_add(dpif, netdev, &port_no,
+                                       &datapath_netdev);
     if (!error) {
         VLOG_DBG_RL(&dpmsg_rl, "%s: added %s as port %"PRIu32,
                     dpif_name(dpif), netdev_name, port_no);
@@ -604,12 +606,12 @@  dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop)
             const char *dpif_type_str = dpif_normalize_type(dpif_type(dpif));
             struct dpif_port dpif_port;
 
-            netdev_set_dpif_type(netdev, dpif_type_str);
+            netdev_set_dpif_type(datapath_netdev, dpif_type_str);
 
             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);
+            netdev_ports_insert(datapath_netdev, &dpif_port);
         }
     } else {
         VLOG_WARN_RL(&error_rl, "%s: failed to add %s as port: %s",