Date:   Tue, 20 Aug 2019 15:40:34 +0300
From:   Paul Blakey <paulb@...lanox.com>
To:     Pravin B Shelar <pshelar@....org>, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Justin Pettit <jpettit@...ira.com>,
        Simon Horman <simon.horman@...ronome.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Vlad Buslov <vladbu@...lanox.com>,
        Paul Blakey <paulb@...lanox.com>
Cc:     Jiri Pirko <jiri@...lanox.com>, Roi Dayan <roid@...lanox.com>,
        Yossi Kuperman <yossiku@...lanox.com>,
        Rony Efraim <ronye@...lanox.com>, Oz Shlomo <ozsh@...lanox.com>
Subject: Re: [PATCH net-next v2] net: openvswitch: Set OvS recirc_id from tc chain index

Regarding the user_features change, I tested the above patch with this one in
userspace that I'll send once this is accepted, togother with the rest
of connection tracking offload patches.

I also have a test for it, if anyone wants it.

Patch is:
lib/netdev-offloads-tc: Probe recirc tc sharing feature on first recirc_id rule

Signed-off-by: Paul Blakey <paulb@...lanox.com>
---
 datapath/linux/compat/include/linux/openvswitch.h |  3 ++
 lib/dpif-netdev.c                                 |  1 +
 lib/dpif-netlink.c                                | 61 +++++++++++++++++++----
 lib/dpif-provider.h                               |  2 +
 lib/dpif.c                                        |  9 ++++
 lib/dpif.h                                        |  2 +
 lib/netdev-offload-tc.c                           | 33 ++++++++++--
 lib/netdev-offload.h                              |  2 +-
 8 files changed, 100 insertions(+), 13 deletions(-)

diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
index 65a003a..921ef5b 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -143,6 +143,9 @@ struct ovs_vport_stats {
 /* Allow datapath to associate multiple Netlink PIDs to each vport */
 #define OVS_DP_F_VPORT_PIDS	(1 << 1)
 
+/* Allow tc offload recirc sharing */
+#define OVS_DP_F_TC_RECIRC_SHARING  (1 << 2)
+
 /* Fixed logical ports. */
 #define OVSP_LOCAL      ((__u32)0)
 
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index d0a1c58..fd8275f 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -7489,6 +7489,7 @@ const struct dpif_class dpif_netdev_class = {
     dpif_netdev_run,
     dpif_netdev_wait,
     dpif_netdev_get_stats,
+    NULL,                      /* set_features */
     dpif_netdev_port_add,
     dpif_netdev_port_del,
     dpif_netdev_port_set_config,
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 07a9ddd..15e6057 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -192,6 +192,7 @@ struct dpif_handler {
 struct dpif_netlink {
     struct dpif dpif;
     int dp_ifindex;
+    uint32_t user_features;
 
     /* Upcall messages. */
     struct fat_rwlock upcall_lock;
@@ -303,7 +304,6 @@ dpif_netlink_enumerate(struct sset *all_dps,
     if (error) {
         return error;
     }
-
     ofpbuf_use_stub(&buf, reply_stub, sizeof reply_stub);
     dpif_netlink_dp_dump_start(&dump);
     while (nl_dump_next(&dump, &msg, &buf)) {
@@ -333,15 +333,26 @@ dpif_netlink_open(const struct dpif_class *class OVS_UNUSED, const char *name,
 
     /* Create or look up datapath. */
     dpif_netlink_dp_init(&dp_request);
+    upcall_pid = 0;
+    dp_request.upcall_pid = &upcall_pid;
+    dp_request.name = name;
+
     if (create) {
         dp_request.cmd = OVS_DP_CMD_NEW;
-        upcall_pid = 0;
-        dp_request.upcall_pid = &upcall_pid;
     } else {
+        dp_request.cmd = OVS_DP_CMD_GET;
+
+        error = dpif_netlink_dp_transact(&dp_request, &dp, &buf);
+        if (error)  {
+            return error;
+        }
+        dp_request.user_features = dp.user_features;
+        ofpbuf_delete(buf);
+
         /* Use OVS_DP_CMD_SET to report user features */
         dp_request.cmd = OVS_DP_CMD_SET;
     }
-    dp_request.name = name;
+
     dp_request.user_features |= OVS_DP_F_UNALIGNED;
     dp_request.user_features |= OVS_DP_F_VPORT_PIDS;
     error = dpif_netlink_dp_transact(&dp_request, &dp, &buf);
@@ -367,6 +378,7 @@ open_dpif(const struct dpif_netlink_dp *dp, struct dpif **dpifp)
               dp->dp_ifindex, dp->dp_ifindex);
 
     dpif->dp_ifindex = dp->dp_ifindex;
+    dpif->user_features = dp->user_features;
     *dpifp = &dpif->dpif;
 
     return 0;
@@ -662,6 +674,31 @@ dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats)
     return error;
 }
 
+static int
+dpif_netlink_set_features(struct dpif *dpif_, uint32_t new_features)
+{
+    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
+    struct dpif_netlink_dp request, reply;
+    struct ofpbuf *bufp;
+    int error;
+
+    dpif_netlink_dp_init(&request);
+    request.cmd = OVS_DP_CMD_SET;
+    request.dp_ifindex = dpif->dp_ifindex;
+    request.user_features = dpif->user_features | new_features;
+
+    error = dpif_netlink_dp_transact(&request, &reply, &bufp);
+    if (!error) {
+        dpif->user_features = reply.user_features;
+        ofpbuf_delete(bufp);
+        if (!(dpif->user_features & new_features)) {
+            return -EOPNOTSUPP;
+        }
+    }
+
+    return error;
+}
+
 static const char *
 get_vport_type(const struct dpif_netlink_vport *vport)
 {
@@ -1989,7 +2026,6 @@ static int
 parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
-    const struct dpif_class *dpif_class = dpif->dpif.dpif_class;
     struct match match;
     odp_port_t in_port;
     const struct nlattr *nla;
@@ -2011,7 +2047,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
     }
 
     in_port = match.flow.in_port.odp_port;
-    dev = netdev_ports_get(in_port, dpif_class);
+    dev = netdev_ports_get(in_port, dpif->dpif.dpif_class);
     if (!dev) {
         return EOPNOTSUPP;
     }
@@ -2024,7 +2060,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
             odp_port_t out_port;
 
             out_port = nl_attr_get_odp_port(nla);
-            outdev = netdev_ports_get(out_port, dpif_class);
+            outdev = netdev_ports_get(out_port, dpif->dpif.dpif_class);
             if (!outdev) {
                 err = EOPNOTSUPP;
                 goto out;
@@ -2040,7 +2076,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
         }
     }
 
-    info.dpif_class = dpif_class;
+    info.dpif = &dpif->dpif;
     info.tp_dst_port = dst_port;
     info.tunnel_csum_on = csum_on;
     err = netdev_flow_put(dev, &match,
@@ -3394,6 +3430,7 @@ const struct dpif_class dpif_netlink_class = {
     dpif_netlink_run,
     NULL,                       /* wait */
     dpif_netlink_get_stats,
+    dpif_netlink_set_features,
     dpif_netlink_port_add,
     dpif_netlink_port_del,
     NULL,                       /* port_set_config */
@@ -3702,6 +3739,9 @@ dpif_netlink_dp_from_ofpbuf(struct dpif_netlink_dp *dp, const struct ofpbuf *buf
         [OVS_DP_ATTR_MEGAFLOW_STATS] = {
                         NL_POLICY_FOR(struct ovs_dp_megaflow_stats),
                         .optional = true },
+        [OVS_DP_ATTR_USER_FEATURES] = {
+                        .type = NL_A_U32,
+                        .optional = true },
     };
 
     dpif_netlink_dp_init(dp);
@@ -3730,6 +3770,10 @@ dpif_netlink_dp_from_ofpbuf(struct dpif_netlink_dp *dp, const struct ofpbuf *buf
         dp->megaflow_stats = nl_attr_get(a[OVS_DP_ATTR_MEGAFLOW_STATS]);
     }
 
+    if (a[OVS_DP_ATTR_USER_FEATURES]) {
+        dp->user_features = nl_attr_get_u32(a[OVS_DP_ATTR_USER_FEATURES]);
+    }
+
     return 0;
 }
 
@@ -3802,7 +3846,6 @@ dpif_netlink_dp_transact(const struct dpif_netlink_dp *request,
     dpif_netlink_dp_to_ofpbuf(request, request_buf);
     error = nl_transact(NETLINK_GENERIC, request_buf, bufp);
     ofpbuf_delete(request_buf);
-
     if (reply) {
         dpif_netlink_dp_init(reply);
         if (!error) {
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 12898b9..8c8bc77 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -187,6 +187,8 @@ struct dpif_class {
     /* Retrieves statistics for 'dpif' into 'stats'. */
     int (*get_stats)(const struct dpif *dpif, struct dpif_dp_stats *stats);
 
+    int (*set_features)(struct dpif *dpif, uint32_t user_features);
+
     /* Adds 'netdev' as a new port in 'dpif'.  If '*port_no' is not
      * ODPP_NONE, attempts to use that as the port's port number.
      *
diff --git a/lib/dpif.c b/lib/dpif.c
index c88b210..dc13655 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -543,6 +543,15 @@ dpif_get_dp_stats(const struct dpif *dpif, struct dpif_dp_stats *stats)
     return error;
 }
 
+int
+dpif_set_features(struct dpif *dpif, uint32_t new_features)
+{
+    int error = dpif->dpif_class->set_features(dpif, new_features);
+
+    log_operation(dpif, "set_features", error);
+    return error;
+}
+
 const char *
 dpif_port_open_type(const char *datapath_type, const char *port_type)
 {
diff --git a/lib/dpif.h b/lib/dpif.h
index 289d574..c7bb48f 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -435,6 +435,8 @@ struct dpif_dp_stats {
 };
 int dpif_get_dp_stats(const struct dpif *, struct dpif_dp_stats *);
 
+int dpif_set_features(struct dpif *, uint32_t new_features);
+
 .
 /* Port operations. */
 
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 60d5a42..4e85585 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -38,6 +38,7 @@
 #include "tc.h"
 #include "unaligned.h"
 #include "util.h"
+#include "dpif-provider.h"
 
 VLOG_DEFINE_THIS_MODULE(netdev_offload_tc);
 
@@ -1354,6 +1355,25 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl,
     flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len;
 }
 
+static bool
+recirc_id_sharing_support(struct dpif *dpif)
+{
+    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
+    static bool supported = false;
+    int err;
+
+    if (ovsthread_once_start(&once)) {
+        err = dpif_set_features(dpif, OVS_DP_F_TC_RECIRC_SHARING);
+        supported = err ? supported : true;
+        if (supported) {
+            VLOG_INFO("probe tc: tc recirc id sharing with OvS datapath is supported.");
+        }
+        ovsthread_once_done(&once);
+    }
+
+    return supported;
+}
+
 static int
 netdev_tc_flow_put(struct netdev *netdev, struct match *match,
                    struct nlattr *actions, size_t actions_len,
@@ -1371,7 +1391,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
     uint32_t block_id = 0;
     struct nlattr *nla;
     struct tc_id id;
-    uint32_t chain;
+    uint32_t chain = 0;
     size_t left;
     int prio = 0;
     int ifindex;
@@ -1386,7 +1406,13 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
 
     memset(&flower, 0, sizeof flower);
 
-    chain = key->recirc_id;
+    if (key->recirc_id) {
+        if (recirc_id_sharing_support(info->dpif)) {
+            chain = key->recirc_id;
+        } else {
+            return -EOPNOTSUPP;
+        }
+    }
     mask->recirc_id = 0;
 
     if (flow_tnl_dst_is_set(&key->tunnel)) {
@@ -1634,7 +1660,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
         action = &flower.actions[flower.action_count];
         if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
             odp_port_t port = nl_attr_get_odp_port(nla);
-            struct netdev *outdev = netdev_ports_get(port, info->dpif_class);
+            struct netdev *outdev = netdev_ports_get(port,
+                                                     info->dpif->dpif_class);
 
             action->out.ifindex_out = netdev_get_ifindex(outdev);
             action->out.ingress = is_internal_port(netdev_get_type(outdev));
diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
index 97a5006..d852fe2 100644
--- a/lib/netdev-offload.h
+++ b/lib/netdev-offload.h
@@ -62,7 +62,7 @@ struct netdev_flow_dump {
 
 /* Flow offloading. */
 struct offload_info {
-    const struct dpif_class *dpif_class;
+    struct dpif *dpif;
     ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */
     uint8_t tunnel_csum_on; /* Tunnel header with checksum */
 
-- 
1.8.3.1

Powered by blists - more mailing lists