Re: [ovs-dev] [PATCH v22 6/8] dpif-netlink: Add netdev offload recv in normal recv upcalls

[Chris Mi via dev]

On 2/23/2023 9:43 PM, Ilya Maximets wrote:

On 2/23/23 12:27, Chris Mi wrote:

In thread handler 0, add netdev offload recv in normal recv upcalls.
To avoid starvation, introduce a flag to alternate the order of
receiving normal upcalls and offload upcalls based on that flag.

Add similar change for recv_wait.

Signed-off-by: Chris Mi<c...@nvidia.com>
Reviewed-by: Roi Dayan<r...@nvidia.com>
---
  lib/dpif-netlink.c            | 46 ++++++++++++++++++++++++++++++++---
  ofproto/ofproto-dpif-upcall.c | 23 +++++++++++++++---
  2 files changed, 62 insertions(+), 7 deletions(-)

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 586fb8893..9f67db1be 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -201,6 +201,12 @@ struct dpif_handler {
      struct nl_sock *sock;         /* Each handler thread holds one netlink
                                       socket. */

+ /* Thread handler 0 deals with both netdev offload recv and normal

+     * recv upcalls. To avoid starvation, introduce a flag to alternate
+     * the order.
+     */
+    bool recv_offload_first;
+
  #ifdef _WIN32
      /* Pool of sockets. */
      struct dpif_windows_vport_sock *vport_sock_pool;
@@ -3130,13 +3136,12 @@ dpif_netlink_recv_vport_dispatch(struct dpif_netlink 
*dpif,
  #endif

static int

-dpif_netlink_recv(struct dpif *dpif_, uint32_t handler_id,
-                  struct dpif_upcall *upcall, struct ofpbuf *buf)
+dpif_netlink_recv__(struct dpif *dpif_, uint32_t handler_id,
+                    struct dpif_upcall *upcall, struct ofpbuf *buf)
  {
      struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
      int error;

- fat_rwlock_rdlock(&dpif->upcall_lock);

  #ifdef _WIN32
      error = dpif_netlink_recv_windows(dpif, handler_id, upcall, buf);
  #else
@@ -3147,6 +3152,38 @@ dpif_netlink_recv(struct dpif *dpif_, uint32_t 
handler_id,
                                                   handler_id, upcall, buf);
      }
  #endif
+
+    return error;
+}
+
+static int
+dpif_netlink_recv(struct dpif *dpif_, uint32_t handler_id,
+                  struct dpif_upcall *upcall, struct ofpbuf *buf)
+{
+    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
+    struct dpif_handler *handler;
+    int error;
+
+    fat_rwlock_rdlock(&dpif->upcall_lock);
+    if (handler_id) {
+        error = dpif_netlink_recv__(dpif_, handler_id, upcall, buf);
+        fat_rwlock_unlock(&dpif->upcall_lock);
+        return error;
+    }
+
+    handler = &dpif->handlers[handler_id];
+    if (handler->recv_offload_first) {
+        error = netdev_offload_recv(upcall, buf);
+        if (error == EAGAIN) {
+            error = dpif_netlink_recv__(dpif_, handler_id, upcall, buf);
+        }
+    } else {
+        error = dpif_netlink_recv__(dpif_, handler_id, upcall, buf);
+        if (error == EAGAIN) {
+            error = netdev_offload_recv(upcall, buf);
+        }
+    }
+    handler->recv_offload_first = !handler->recv_offload_first;
      fat_rwlock_unlock(&dpif->upcall_lock);

return error;

@@ -3211,6 +3248,9 @@ dpif_netlink_recv_wait(struct dpif *dpif_, uint32_t 
handler_id)
      } else {
          dpif_netlink_recv_wait_vport_dispatch(dpif, handler_id);
      }
+    if (handler_id == 0) {
+        netdev_offload_recv_wait();
+    }
  #endif
      fat_rwlock_unlock(&dpif->upcall_lock);
  }
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index fc94078cb..273b576bd 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -840,10 +840,25 @@ recv_upcalls(struct handler *handler)
              break;
          }

- upcall->fitness = odp_flow_key_to_flow(dupcall->key, dupcall->key_len,

-                                               flow, NULL);
-        if (upcall->fitness == ODP_FIT_ERROR) {
-            goto free_dupcall;
+        /* If it is normal upcalls, datapath will provide key and key_len
+         * to construct flow. But for netdev offload upcalls, key and
+         * key_len are not available. Construct partial flow using available
+         * info.> +         */
+        if (dupcall->key && dupcall->key_len) {
+            upcall->fitness = odp_flow_key_to_flow(dupcall->key,
+                                                   dupcall->key_len,
+                                                   flow, NULL);
+            if (upcall->fitness == ODP_FIT_ERROR) {
+                goto free_dupcall;
+            }
+        } else {
+            memset(flow, 0, sizeof *flow);
+            if (dupcall->in_tun) {
+                memcpy(&flow->tunnel, dupcall->in_tun, sizeof flow->tunnel);
+            }
+            flow->in_port.odp_port =
+                netdev_ifindex_to_odp_port(dupcall->iifindex);

I didn't read the whole set, but this doesn't look right.
In particular, calling netdev_ifindex_to_odp_port() from the generic
upcall processing code.

I agree though that it doesn't make a lot of sense to have a key/key_len
for an sFlow upcall.  A better approach would be to add an actual struct
flow to the dupcall structure.  You're already adding a tunnel metadata
there, we could just add a full flow instead and make offload layer to
prepare it.

In that case recv_upcalls() may check if it has key to parse or it already
has a parsed flow structure.

No HWOL-specific calls like netdev_ifindex_to_odp_port() will be needed
in the generic code then.  And it will be netdev-offload layer's
responsibility to properly fill in a flow structure.

That's a good suggestion. I added struct flow in struct dpif_upcall.

P.S. many patches in this patch set are failing thread-safety analysis.

Maybe you mean OVS_REQ_RDLOCK(dpif->upcall_lock), I fixed it in v23.

Best regards, Ilya Maximets.

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev