diff mbox series

[ovs-dev,v3,05/10] dpif: Add support to set user features

Message ID 20191203134534.32056-6-roid@mellanox.com
State Superseded
Headers show
Series Add support for offloading CT datapath rules to TC | expand

Commit Message

Roi Dayan Dec. 3, 2019, 1:45 p.m. UTC 
From: Paul Blakey <paulb@mellanox.com>

This enables user features on the kernel datapath via the DP_CMD_SET
command, and also retrieves them to check for actual support and
not just an older kernel ignoring the requested features.

This will be used in next patch to enable recirc_id sharing with tc.

----

Changelog:

V2->V3:
    Refactored commit, to move it earlier
    Renamed commit from "netdev-offloads-tc: Probe recirc tc sharing feature on first recirc_id rule"

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

Comments

0-day Robot Dec. 3, 2019, 2:09 p.m. UTC | #1 
Bleep bloop.  Greetings Roi Dayan, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


build:
depbase=`echo lib/dpif-netlink-rtnl.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -MT lib/dpif-netlink-rtnl.lo -MD -MP -MF $depbase.Tpo -c -o lib/dpif-netlink-rtnl.lo lib/dpif-netlink-rtnl.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/dpif-netlink-rtnl.lo -MD -MP -MF lib/.deps/dpif-netlink-rtnl.Tpo -c lib/dpif-netlink-rtnl.c -o lib/dpif-netlink-rtnl.o
depbase=`echo lib/if-notifier.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -MT lib/if-notifier.lo -MD -MP -MF $depbase.Tpo -c -o lib/if-notifier.lo lib/if-notifier.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/if-notifier.lo -MD -MP -MF lib/.deps/if-notifier.Tpo -c lib/if-notifier.c -o lib/if-notifier.o
depbase=`echo lib/netdev-linux.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -MT lib/netdev-linux.lo -MD -MP -MF $depbase.Tpo -c -o lib/netdev-linux.lo lib/netdev-linux.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/netdev-linux.lo -MD -MP -MF lib/.deps/netdev-linux.Tpo -c lib/netdev-linux.c -o lib/netdev-linux.o
depbase=`echo lib/netdev-offload-tc.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool  --tag=CC   --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I.    -I ./include -I ./include -I ./lib -I ./lib    -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror   -g -O2 -MT lib/netdev-offload-tc.lo -MD -MP -MF $depbase.Tpo -c -o lib/netdev-offload-tc.lo lib/netdev-offload-tc.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/netdev-offload-tc.lo -MD -MP -MF lib/.deps/netdev-offload-tc.Tpo -c lib/netdev-offload-tc.c -o lib/netdev-offload-tc.o
lib/netdev-offload-tc.c: In function 'netdev_tc_flow_put':
lib/netdev-offload-tc.c:1371:64: error: dereferencing pointer to incomplete type
                                                      info->dpif->dpif_class);
                                                                ^
make[2]: *** [lib/netdev-offload-tc.lo] Error 1
make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Simon Horman Dec. 11, 2019, 7:55 p.m. UTC | #2 
On Tue, Dec 03, 2019 at 03:45:29PM +0200, Roi Dayan wrote:
> From: Paul Blakey <paulb@mellanox.com>
> 
> This enables user features on the kernel datapath via the DP_CMD_SET
> command, and also retrieves them to check for actual support and
> not just an older kernel ignoring the requested features.
> 
> This will be used in next patch to enable recirc_id sharing with tc.
> 
> ----
> 
> Changelog:
> 
> V2->V3:
>     Refactored commit, to move it earlier
>     Renamed commit from "netdev-offloads-tc: Probe recirc tc sharing feature on first recirc_id rule"
> 
> Signed-off-by: Paul Blakey <paulb@mellanox.com>

I think this has been reported elsewhere, but there seems to be a build
error introduced by this patch.

https://patchwork.ozlabs.org/patch/1203712/#2316709

https://travis-ci.org/horms2/ovs/jobs/623718979
Oz Shlomo Dec. 15, 2019, 10:04 a.m. UTC | #3 
+Paul Blakey

On 12/11/2019 9:55 PM, Simon Horman wrote:
> On Tue, Dec 03, 2019 at 03:45:29PM +0200, Roi Dayan wrote:
>> From: Paul Blakey <paulb@mellanox.com>
>>
>> This enables user features on the kernel datapath via the DP_CMD_SET
>> command, and also retrieves them to check for actual support and
>> not just an older kernel ignoring the requested features.
>>
>> This will be used in next patch to enable recirc_id sharing with tc.
>>
>> ----
>>
>> Changelog:
>>
>> V2->V3:
>>      Refactored commit, to move it earlier
>>      Renamed commit from "netdev-offloads-tc: Probe recirc tc sharing feature on first recirc_id rule"
>>
>> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> I think this has been reported elsewhere, but there seems to be a build
> error introduced by this patch.
>
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F1203712%2F%232316709&amp;data=02%7C01%7Cozsh%40mellanox.com%7C724df8013d224569e1d308d77e741010%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C637116909353848346&amp;sdata=iF6tuZ06TURpB%2FwoU%2F9FZDFBUc0QlnJxPHf27CSj3uQ%3D&amp;reserved=0
>
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fhorms2%2Fovs%2Fjobs%2F623718979&amp;data=02%7C01%7Cozsh%40mellanox.com%7C724df8013d224569e1d308d77e741010%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C1%7C637116909353858343&amp;sdata=PqNAcEYGQ4V4Tl2o4WHDx9%2F1mlTCjPvgaIGk9mU7o%2Bw%3D&amp;reserved=0
>
>
diff mbox series

Patch

diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
index 778827f8b5a2..b9a7faa254ef 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 5142bad1dbd5..5886a75d0f14 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -7539,6 +7539,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 e9a6887f7af2..92da918544d1 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -193,6 +193,7 @@  struct dpif_handler {
 struct dpif_netlink {
     struct dpif dpif;
     int dp_ifindex;
+    uint32_t user_features;
 
     /* Upcall messages. */
     struct fat_rwlock upcall_lock;
@@ -334,15 +335,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);
@@ -368,6 +380,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;
@@ -664,6 +677,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)
 {
@@ -1990,7 +2028,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;
@@ -2012,7 +2049,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;
     }
@@ -2025,7 +2062,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;
@@ -2041,7 +2078,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,
@@ -3885,6 +3922,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 */
@@ -4202,6 +4240,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);
@@ -4230,6 +4271,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;
 }
 
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index f8ca310d0fa5..b77317bca18b 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -188,6 +188,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 c88b2106f086..dc13655aa832 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 c98513e48cc7..6c933b24113f 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 cdf7c61b5505..bb0eccd79ceb 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1367,7 +1367,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 97a50064715d..d852fe2acd90 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 */