diff mbox series

[ovs-dev,v7,1/3] dpif-netlink: Detect Out-Of-Resource condition on a netdev

Message ID 20180927064827.26256-2-sriharsha.basavapatna@broadcom.com
State Deferred
Headers show
Series Support dynamic rebalancing of offloaded flows | expand

Commit Message

Li,Rongqing via dev Sept. 27, 2018, 6:48 a.m. UTC 
This is the first patch in the patch-set to support dynamic rebalancing
of offloaded flows.

The patch detects OOR condition on a netdev port when ENOSPC error is
returned by TC-Flower while adding a flow rule. A new structure is added
to the netdev called "netdev_hw_info", to store OOR related information
required to perform dynamic offload-rebalancing.

Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Co-authored-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
Reviewed-by: Sathya Perla <sathya.perla@broadcom.com>
---
 lib/dpif-netlink.c    | 18 +++++++++++++++++-
 lib/flow.c            | 25 +++++++++++++++++++++++++
 lib/flow.h            |  1 +
 lib/netdev-provider.h | 11 +++++++++++
 lib/netdev.c          | 35 +++++++++++++++++++++++++++++++++++
 lib/netdev.h          |  3 +++
 6 files changed, 92 insertions(+), 1 deletion(-)

Comments

0-day Robot Sept. 27, 2018, 6:55 a.m. UTC | #1 
Bleep bloop.  Greetings Sriharsha Basavapatna via dev, 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.


checkpatch:
ERROR: Author Sriharsha Basavapatna via dev <ovs-dev@openvswitch.org> needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Lines checked: 223, Warnings: 1, Errors: 1


build:
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 ofproto/ofproto_libofproto_la-netflow.lo -MD -MP -MF ofproto/.deps/ofproto_libofproto_la-netflow.Tpo -c ofproto/netflow.c -o ofproto/ofproto_libofproto_la-netflow.o
mv -f ofproto/.deps/ofproto_libofproto_la-netflow.Tpo ofproto/.deps/ofproto_libofproto_la-netflow.Plo
/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 ofproto/ofproto_libofproto_la-ofproto.lo -MD -MP -MF ofproto/.deps/ofproto_libofproto_la-ofproto.Tpo -c -o ofproto/ofproto_libofproto_la-ofproto.lo `test -f 'ofproto/ofproto.c' || echo './'`ofproto/ofproto.c
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 ofproto/ofproto_libofproto_la-ofproto.lo -MD -MP -MF ofproto/.deps/ofproto_libofproto_la-ofproto.Tpo -c ofproto/ofproto.c -o ofproto/ofproto_libofproto_la-ofproto.o
ofproto/ofproto.c: In function 'ofproto_group_mod_finish':
ofproto/ofproto.c:7400:13: error: missing initializer for field 'new_buckets' of 'struct <anonymous>' [-Werror=missing-field-initializers]
             .new_buckets = new_group ? &new_group->buckets : NULL,
             ^
In file included from ofproto/ofproto.c:48:0:
./include/openvswitch/ofp-monitor.h:135:36: note: 'new_buckets' declared here
             const struct ovs_list *new_buckets;
                                    ^
ofproto/ofproto.c:7401:13: error: missing initializer for field 'group_existed' of 'struct <anonymous>' [-Werror=missing-field-initializers]
             .group_existed = group_collection_n(&ogm->old_groups) > 0,
             ^
In file included from ofproto/ofproto.c:48:0:
./include/openvswitch/ofp-monitor.h:140:17: note: 'group_existed' declared here
             int group_existed;
                 ^
cc1: all warnings being treated as errors
make[2]: *** [ofproto/ofproto_libofproto_la-ofproto.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@bytheb.org

Thanks,
0-day Robot
Simon Horman Oct. 12, 2018, 9:32 a.m. UTC | #2 
On Thu, Sep 27, 2018 at 12:18:25PM +0530, Sriharsha Basavapatna via dev wrote:
> This is the first patch in the patch-set to support dynamic rebalancing
> of offloaded flows.
> 
> The patch detects OOR condition on a netdev port when ENOSPC error is
> returned by TC-Flower while adding a flow rule. A new structure is added
> to the netdev called "netdev_hw_info", to store OOR related information
> required to perform dynamic offload-rebalancing.
> 
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> Co-authored-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> Reviewed-by: Sathya Perla <sathya.perla@broadcom.com>
> ---
>  lib/dpif-netlink.c    | 18 +++++++++++++++++-
>  lib/flow.c            | 25 +++++++++++++++++++++++++
>  lib/flow.h            |  1 +
>  lib/netdev-provider.h | 11 +++++++++++
>  lib/netdev.c          | 35 +++++++++++++++++++++++++++++++++++
>  lib/netdev.h          |  3 +++
>  6 files changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index e6d5a6ec5..b9ce9cbe2 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -2178,7 +2178,23 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
>  
>          VLOG_DBG("added flow");
>      } else if (err != EEXIST) {
> -        VLOG_ERR_RL(&rl, "failed to offload flow: %s", ovs_strerror(err));
> +        struct netdev *oor_netdev = NULL;
> +        if (err == ENOSPC && netdev_is_offload_rebalance_policy_enabled()) {
> +            /*
> +             * We need to set OOR on the input netdev (i.e, 'dev') for the
> +             * flow. But if the flow has a tunnel attribute (i.e, decap action,
> +             * with a virtual device like a VxLAN interface as its in-port),
> +             * then lookup and set OOR on the underlying tunnel (real) netdev.
> +             */
> +            oor_netdev = flow_get_tunnel_netdev(&match.flow.tunnel);
>
> +            if (!oor_netdev) {
> +                /* Not a 'tunnel' flow */
> +                oor_netdev = dev;
> +            }
> +            netdev_set_hw_info(oor_netdev, HW_INFO_TYPE_OOR, true);
> +        }
> +        VLOG_ERR_RL(&rl, "failed to offload flow: %s: %s", ovs_strerror(err),
> +                    (oor_netdev ? oor_netdev->name : dev->name));
>      }
>  
>  out:
> diff --git a/lib/flow.c b/lib/flow.c
> index 77ed3d9df..a39807908 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -19,6 +19,7 @@
>  #include <errno.h>
>  #include <inttypes.h>
>  #include <limits.h>
> +#include <net/if.h>
>  #include <netinet/in.h>
>  #include <netinet/icmp6.h>
>  #include <netinet/ip6.h>
> @@ -41,6 +42,8 @@
>  #include "unaligned.h"
>  #include "util.h"
>  #include "openvswitch/nsh.h"
> +#include "ovs-router.h"
> +#include "lib/netdev-provider.h"
>  
>  COVERAGE_DEFINE(flow_extract);
>  COVERAGE_DEFINE(miniflow_malloc);
> @@ -3403,3 +3406,25 @@ flow_limit_vlans(int vlan_limit)
>          flow_vlan_limit = MIN(vlan_limit, FLOW_MAX_VLAN_HEADERS);
>      }
>  }
> +
> +struct netdev *
> +flow_get_tunnel_netdev(struct flow_tnl *tunnel)
> +{
> +    char iface[IFNAMSIZ];
> +    struct in6_addr ip6;
> +    struct in6_addr gw;
> +
> +    if (tunnel->ip_src) {
> +        in6_addr_set_mapped_ipv4(&ip6, tunnel->ip_src);
> +    } else if (ipv6_addr_is_set(&tunnel->ipv6_src)) {
> +        ip6 = tunnel->ipv6_src;
> +    } else {
> +        return NULL;
> +    }
> +
> +    if (!ovs_router_lookup(0, &ip6, iface, NULL, &gw)) {
> +        return NULL;
> +    }

I think what is desired here is the offload device of the flow.
In the case of a tunnel, its not the tunnel netdev. And it seems
this function tries to find the offload device by looking at the
egress route for the tunnel. But what if device of that route is a bridge,
LAG or other upper netdev?

I don't think this problem needs to block progress of this patchset.
But it may be an interesting area for further work.

> +
> +    return netdev_from_name(iface);
> +}
> diff --git a/lib/flow.h b/lib/flow.h
> index d03f1ba9c..aca60c41a 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -73,6 +73,7 @@ void flow_extract(struct dp_packet *, struct flow *);
>  void flow_zero_wildcards(struct flow *, const struct flow_wildcards *);
>  void flow_unwildcard_tp_ports(const struct flow *, struct flow_wildcards *);
>  void flow_get_metadata(const struct flow *, struct match *flow_metadata);
> +struct netdev *flow_get_tunnel_netdev(struct flow_tnl *tunnel);
>  
>  const char *ct_state_to_string(uint32_t state);
>  uint32_t ct_state_from_string(const char *);
> diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
> index 5a7947351..e320dad61 100644
> --- a/lib/netdev-provider.h
> +++ b/lib/netdev-provider.h
> @@ -35,6 +35,15 @@ extern "C" {
>  struct netdev_tnl_build_header_params;
>  #define NETDEV_NUMA_UNSPEC OVS_NUMA_UNSPEC
>  
> +/* Offload-capable (HW) netdev information */
> +struct netdev_hw_info {
> +    bool oor;		/* Out of Offload Resources ? */
> +};
> +
> +enum hw_info_type {
> +    HW_INFO_TYPE_OOR = 1	/* OOR state */
> +};
> +
>  /* A network device (e.g. an Ethernet device).
>   *
>   * Network device implementations may read these members but should not modify
> @@ -80,6 +89,8 @@ struct netdev {
>      int n_rxq;
>      struct shash_node *node;            /* Pointer to element in global map. */
>      struct ovs_list saved_flags_list; /* Contains "struct netdev_saved_flags". */
> +
> +    struct netdev_hw_info hw_info;	/* offload-capable netdev info */
>  };
>  
>  static inline void
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 690d28409..3e2ce7c09 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -415,6 +415,7 @@ netdev_open(const char *name, const char *type, struct netdev **netdevp)
>                  netdev->reconfigure_seq = seq_create();
>                  netdev->last_reconfigure_seq =
>                      seq_read(netdev->reconfigure_seq);
> +                netdev->hw_info.oor = false;
>                  netdev->node = shash_add(&netdev_shash, name, netdev);
>  
>                  /* By default enable one tx and rx queue per netdev. */
> @@ -2252,6 +2253,31 @@ netdev_get_block_id(struct netdev *netdev)
>              : 0);
>  }
>  
> +/*
> + * Get the value of the hw info parameter specified by type.
> + * Returns the value on success (>= 0). Returns -1 on failure.
> + */
> +int
> +netdev_get_hw_info(struct netdev *netdev, int type)
> +{
> +    if (type == HW_INFO_TYPE_OOR) {
> +        return netdev->hw_info.oor;
> +    }
> +
> +    return -1;
> +}
> +
> +/*
> + * Set the value of the hw info parameter specified by type.
> + */
> +void
> +netdev_set_hw_info(struct netdev *netdev, int type, int val)
> +{
> +    if (type == HW_INFO_TYPE_OOR) {
> +        netdev->hw_info.oor = val;
> +    }
> +}
> +
>  bool
>  netdev_is_flow_api_enabled(void)
>  {
> @@ -2489,6 +2515,15 @@ netdev_free_custom_stats_counters(struct netdev_custom_stats *custom_stats)
>  }
>  
>  #ifdef __linux__
> +
> +static bool netdev_offload_rebalance_policy = false;
> +
> +bool
> +netdev_is_offload_rebalance_policy_enabled(void)
> +{
> +    return netdev_offload_rebalance_policy;
> +}
> +
>  static void
>  netdev_ports_flow_init(void)
>  {
> diff --git a/lib/netdev.h b/lib/netdev.h
> index 556676046..b0e5c5b72 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -227,8 +227,11 @@ int netdev_flow_del(struct netdev *, const ovs_u128 *,
>                      struct dpif_flow_stats *);
>  int netdev_init_flow_api(struct netdev *);
>  uint32_t netdev_get_block_id(struct netdev *);
> +int netdev_get_hw_info(struct netdev *, int);
> +void netdev_set_hw_info(struct netdev *, int, int);
>  bool netdev_is_flow_api_enabled(void);
>  void netdev_set_flow_api_enabled(const struct smap *ovs_other_config);
> +bool netdev_is_offload_rebalance_policy_enabled(void);
>  
>  struct dpif_port;
>  int netdev_ports_insert(struct netdev *, const struct dpif_class *,
> -- 
> 2.18.0.rc1.1.g6f333ff
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index e6d5a6ec5..b9ce9cbe2 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2178,7 +2178,23 @@  parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
 
         VLOG_DBG("added flow");
     } else if (err != EEXIST) {
-        VLOG_ERR_RL(&rl, "failed to offload flow: %s", ovs_strerror(err));
+        struct netdev *oor_netdev = NULL;
+        if (err == ENOSPC && netdev_is_offload_rebalance_policy_enabled()) {
+            /*
+             * We need to set OOR on the input netdev (i.e, 'dev') for the
+             * flow. But if the flow has a tunnel attribute (i.e, decap action,
+             * with a virtual device like a VxLAN interface as its in-port),
+             * then lookup and set OOR on the underlying tunnel (real) netdev.
+             */
+            oor_netdev = flow_get_tunnel_netdev(&match.flow.tunnel);
+            if (!oor_netdev) {
+                /* Not a 'tunnel' flow */
+                oor_netdev = dev;
+            }
+            netdev_set_hw_info(oor_netdev, HW_INFO_TYPE_OOR, true);
+        }
+        VLOG_ERR_RL(&rl, "failed to offload flow: %s: %s", ovs_strerror(err),
+                    (oor_netdev ? oor_netdev->name : dev->name));
     }
 
 out:
diff --git a/lib/flow.c b/lib/flow.c
index 77ed3d9df..a39807908 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -19,6 +19,7 @@ 
 #include <errno.h>
 #include <inttypes.h>
 #include <limits.h>
+#include <net/if.h>
 #include <netinet/in.h>
 #include <netinet/icmp6.h>
 #include <netinet/ip6.h>
@@ -41,6 +42,8 @@ 
 #include "unaligned.h"
 #include "util.h"
 #include "openvswitch/nsh.h"
+#include "ovs-router.h"
+#include "lib/netdev-provider.h"
 
 COVERAGE_DEFINE(flow_extract);
 COVERAGE_DEFINE(miniflow_malloc);
@@ -3403,3 +3406,25 @@  flow_limit_vlans(int vlan_limit)
         flow_vlan_limit = MIN(vlan_limit, FLOW_MAX_VLAN_HEADERS);
     }
 }
+
+struct netdev *
+flow_get_tunnel_netdev(struct flow_tnl *tunnel)
+{
+    char iface[IFNAMSIZ];
+    struct in6_addr ip6;
+    struct in6_addr gw;
+
+    if (tunnel->ip_src) {
+        in6_addr_set_mapped_ipv4(&ip6, tunnel->ip_src);
+    } else if (ipv6_addr_is_set(&tunnel->ipv6_src)) {
+        ip6 = tunnel->ipv6_src;
+    } else {
+        return NULL;
+    }
+
+    if (!ovs_router_lookup(0, &ip6, iface, NULL, &gw)) {
+        return NULL;
+    }
+
+    return netdev_from_name(iface);
+}
diff --git a/lib/flow.h b/lib/flow.h
index d03f1ba9c..aca60c41a 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -73,6 +73,7 @@  void flow_extract(struct dp_packet *, struct flow *);
 void flow_zero_wildcards(struct flow *, const struct flow_wildcards *);
 void flow_unwildcard_tp_ports(const struct flow *, struct flow_wildcards *);
 void flow_get_metadata(const struct flow *, struct match *flow_metadata);
+struct netdev *flow_get_tunnel_netdev(struct flow_tnl *tunnel);
 
 const char *ct_state_to_string(uint32_t state);
 uint32_t ct_state_from_string(const char *);
diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
index 5a7947351..e320dad61 100644
--- a/lib/netdev-provider.h
+++ b/lib/netdev-provider.h
@@ -35,6 +35,15 @@  extern "C" {
 struct netdev_tnl_build_header_params;
 #define NETDEV_NUMA_UNSPEC OVS_NUMA_UNSPEC
 
+/* Offload-capable (HW) netdev information */
+struct netdev_hw_info {
+    bool oor;		/* Out of Offload Resources ? */
+};
+
+enum hw_info_type {
+    HW_INFO_TYPE_OOR = 1	/* OOR state */
+};
+
 /* A network device (e.g. an Ethernet device).
  *
  * Network device implementations may read these members but should not modify
@@ -80,6 +89,8 @@  struct netdev {
     int n_rxq;
     struct shash_node *node;            /* Pointer to element in global map. */
     struct ovs_list saved_flags_list; /* Contains "struct netdev_saved_flags". */
+
+    struct netdev_hw_info hw_info;	/* offload-capable netdev info */
 };
 
 static inline void
diff --git a/lib/netdev.c b/lib/netdev.c
index 690d28409..3e2ce7c09 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -415,6 +415,7 @@  netdev_open(const char *name, const char *type, struct netdev **netdevp)
                 netdev->reconfigure_seq = seq_create();
                 netdev->last_reconfigure_seq =
                     seq_read(netdev->reconfigure_seq);
+                netdev->hw_info.oor = false;
                 netdev->node = shash_add(&netdev_shash, name, netdev);
 
                 /* By default enable one tx and rx queue per netdev. */
@@ -2252,6 +2253,31 @@  netdev_get_block_id(struct netdev *netdev)
             : 0);
 }
 
+/*
+ * Get the value of the hw info parameter specified by type.
+ * Returns the value on success (>= 0). Returns -1 on failure.
+ */
+int
+netdev_get_hw_info(struct netdev *netdev, int type)
+{
+    if (type == HW_INFO_TYPE_OOR) {
+        return netdev->hw_info.oor;
+    }
+
+    return -1;
+}
+
+/*
+ * Set the value of the hw info parameter specified by type.
+ */
+void
+netdev_set_hw_info(struct netdev *netdev, int type, int val)
+{
+    if (type == HW_INFO_TYPE_OOR) {
+        netdev->hw_info.oor = val;
+    }
+}
+
 bool
 netdev_is_flow_api_enabled(void)
 {
@@ -2489,6 +2515,15 @@  netdev_free_custom_stats_counters(struct netdev_custom_stats *custom_stats)
 }
 
 #ifdef __linux__
+
+static bool netdev_offload_rebalance_policy = false;
+
+bool
+netdev_is_offload_rebalance_policy_enabled(void)
+{
+    return netdev_offload_rebalance_policy;
+}
+
 static void
 netdev_ports_flow_init(void)
 {
diff --git a/lib/netdev.h b/lib/netdev.h
index 556676046..b0e5c5b72 100644
--- a/lib/netdev.h
+++ b/lib/netdev.h
@@ -227,8 +227,11 @@  int netdev_flow_del(struct netdev *, const ovs_u128 *,
                     struct dpif_flow_stats *);
 int netdev_init_flow_api(struct netdev *);
 uint32_t netdev_get_block_id(struct netdev *);
+int netdev_get_hw_info(struct netdev *, int);
+void netdev_set_hw_info(struct netdev *, int, int);
 bool netdev_is_flow_api_enabled(void);
 void netdev_set_flow_api_enabled(const struct smap *ovs_other_config);
+bool netdev_is_offload_rebalance_policy_enabled(void);
 
 struct dpif_port;
 int netdev_ports_insert(struct netdev *, const struct dpif_class *,