Message ID | 20180708134540.40547-2-sriharsha.basavapatna@broadcom.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Support dynamic rebalancing of offloaded flows | expand |
On Sun, Jul 08, 2018 at 07:15:37PM +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> Thanks for the patch. This fails to build on my system, with: In file included from ../lib/lldp/lldpd.h:32, from ../lib/ovs-lldp.h:26, from ../ofproto/ofproto-dpif-xlate.h:28, from ../ofproto/ofproto-dpif-upcall.c:36: ../ofproto/ofproto-dpif-upcall.c: In function 'udpif_update_flow_pps': ../ofproto/ofproto-dpif-upcall.c:2630:18: error: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'long long unsigned int' [-Werror=format=] ../include/openvswitch/vlog.h:277:41: note: in definition of macro 'VLOG' ../ofproto/ofproto-dpif-upcall.c:2630:9: note: in expansion of macro 'VLOG_DBG' ../ofproto/ofproto-dpif-upcall.c:2630:18: error: format '%lu' expects argument of type 'long unsigned int', but argument 6 has type 'uint64_t' {aka 'long long unsigned int'} [-Werror=format=] ../include/openvswitch/vlog.h:277:41: note: in definition of macro 'VLOG' ../ofproto/ofproto-dpif-upcall.c:2630:9: note: in expansion of macro 'VLOG_DBG' I guess that you should use "%llu" for unsigned long long, and "%"PRIu64 for uint64_t. Alternatively, a lot of the debug logging here doesn't seem particularly useful, should it be there? (Often __func__ is a sign that a log message is more of a debug aid for the programmer and not something that should appear in the field.)
On Tue, Jul 10, 2018 at 4:13 AM, Ben Pfaff <blp@ovn.org> wrote: > On Sun, Jul 08, 2018 at 07:15:37PM +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> > > Thanks for the patch. > > This fails to build on my system, with: > > In file included from ../lib/lldp/lldpd.h:32, > from ../lib/ovs-lldp.h:26, > from ../ofproto/ofproto-dpif-xlate.h:28, > from ../ofproto/ofproto-dpif-upcall.c:36: > ../ofproto/ofproto-dpif-upcall.c: In function 'udpif_update_flow_pps': > ../ofproto/ofproto-dpif-upcall.c:2630:18: error: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'long long unsigned int' [-Werror=format=] > ../include/openvswitch/vlog.h:277:41: note: in definition of macro 'VLOG' > ../ofproto/ofproto-dpif-upcall.c:2630:9: note: in expansion of macro 'VLOG_DBG' > ../ofproto/ofproto-dpif-upcall.c:2630:18: error: format '%lu' expects argument of type 'long unsigned int', but argument 6 has type 'uint64_t' {aka 'long long unsigned int'} [-Werror=format=] > ../include/openvswitch/vlog.h:277:41: note: in definition of macro 'VLOG' > ../ofproto/ofproto-dpif-upcall.c:2630:9: note: in expansion of macro 'VLOG_DBG' > > I guess that you should use "%llu" for unsigned long long, and "%"PRIu64 > for uint64_t. Alternatively, a lot of the debug logging here doesn't > seem particularly useful, should it be there? (Often __func__ is a sign > that a log message is more of a debug aid for the programmer and not > something that should appear in the field.) I agree, this one was more of a debug aid. I'll remove it and revisit the other ones that we added. Thanks, -Harsha
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index aa9bbd946..5a6d53ad5 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -2097,11 +2097,16 @@ 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 netdev_hw_info *hw_info; struct match match; odp_port_t in_port; + odp_port_t out_port; const struct nlattr *nla; size_t left; struct netdev *dev; + struct netdev *outdev = NULL; + struct netdev *tunnel_netdev = NULL; + struct netdev *oor_netdev = NULL; struct offload_info info; ovs_be16 dst_port = 0; int err; @@ -2131,8 +2136,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) { if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) { const struct netdev_tunnel_config *tnl_cfg; - struct netdev *outdev; - odp_port_t out_port; out_port = nl_attr_get_odp_port(nla); outdev = netdev_ports_get(out_port, dpif_class); @@ -2144,7 +2147,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put) if (tnl_cfg && tnl_cfg->dst_port != 0) { dst_port = tnl_cfg->dst_port; } - netdev_close(outdev); } } @@ -2175,7 +2177,18 @@ 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)); + if (outdev && dev && (err == ENOSPC)) { + tunnel_netdev = flow_get_tunnel_netdev(&match.flow.tunnel); + if (tunnel_netdev) { + oor_netdev = tunnel_netdev; + } else { + oor_netdev = dev; + } + hw_info = &oor_netdev->hw_info; + hw_info->oor = true; + } + VLOG_ERR_RL(&rl, "failed to offload flow: %s: %s", ovs_strerror(err), + (oor_netdev ? oor_netdev->name : dev->name)); } out: @@ -2196,6 +2209,7 @@ out: } } + netdev_close(outdev); netdev_close(dev); return err; diff --git a/lib/flow.c b/lib/flow.c index 75ca45672..912afc99d 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); @@ -3299,3 +3302,27 @@ 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) +{ + struct netdev *tunnel_netdev; + 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); + } + + tunnel_netdev = netdev_from_name(iface); + return tunnel_netdev; +} diff --git a/lib/flow.h b/lib/flow.h index 5b6585f11..a67abc9c9 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 1a572f5b8..62e05619e 100644 --- a/lib/netdev-provider.h +++ b/lib/netdev-provider.h @@ -35,6 +35,11 @@ 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 ? */ +}; + /* A network device (e.g. an Ethernet device). * * Network device implementations may read these members but should not modify @@ -80,6 +85,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 82ffeb901..b17f0563f 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. */ @@ -2472,6 +2473,7 @@ netdev_free_custom_stats_counters(struct netdev_custom_stats *custom_stats) } #ifdef __linux__ + static void netdev_ports_flow_init(void) {