[ovs-dev] [loops 6/6] datapath: Detect and suppress flows that are implicated in loops.

Ben Pfaff blp at nicira.com
Fri Jul 23 23:44:58 UTC 2010


In-kernel loop need to be suppressed; otherwise, they cause high CPU
consumption, even to the point that the machine becomes unusable.  Ideally,
these flows should never be added to the Open vSwitch flow table, but it
is fairly easy for a buggy controller to create them given the menagerie
of tunnels, patches, etc. that OVS makes available.

Commit ecbb6953b "datapath: Add loop checking" did the initial work
toward suppressing loops, by dropping packets that recursed more than 5
times.  This at least prevented the kernel stack from overflowing and
thereby OOPSing the machine.  But even with this commit, it is still
possible to waste a lot of CPU time due to loops.  The problem is not
limited to 5 recursive calls per packet: any packet can be sent to
multiple destinations, which in turn can themselves be sent to multiple
destinations, and so on.  We have actually seen in practice a case where
each packet was, apparently, sent to at least 2 destinations per hop, so
that each packet actually consumed CPU time for 2**5 == 32 packets,
possibly more.

This commit takes loop suppression a step further, by clearing the actions
of flows that are implicated in loops.  Thus, after the first packet in
such a flow, later packets for either the "root" flow or for flows that
it ends up looping through are simply discarded, saving a huge amount of
CPU time.

I didn't have much luck reproducing the worst case in my VM test setup, but
I did show a significant improvement in performance with simpler test cases
that I was able to construct.

This version of the commit just clears the actions from the flows that are
part of the loop.  Probably, there should be some additional action to tell
ovs-vswitchd that a loop has been detected, so that it can in turn inform
the controller one way or another.
---
 datapath/datapath.c |    8 ++++++--
 datapath/vport.c    |   29 ++++++++++++++++++-----------
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index e05608a..b233cd2 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -539,9 +539,13 @@ void dp_process_received_packet(struct dp_port *p, struct sk_buff *skb)
 	if (flow_node) {
 		struct sw_flow *flow = flow_cast(flow_node);
 		struct sw_flow_actions *acts = rcu_dereference(flow->sf_acts);
+		int err;
+
 		flow_used(flow, skb);
-		execute_actions(dp, skb, &key, acts->actions, acts->n_actions,
-				GFP_ATOMIC);
+		err = execute_actions(dp, skb, &key, acts->actions,
+				      acts->n_actions, GFP_ATOMIC);
+		if (err == -ELOOP)
+			acts->n_actions = 0;
 		stats_counter_off = offsetof(struct dp_stats_percpu, n_hit);
 	} else {
 		stats_counter_off = offsetof(struct dp_stats_percpu, n_missed);
diff --git a/datapath/vport.c b/datapath/vport.c
index 862b295..42a83e5 100644
--- a/datapath/vport.c
+++ b/datapath/vport.c
@@ -36,14 +36,19 @@ static struct hlist_head *dev_table;
 #define VPORT_HASH_BUCKETS 1024
 
 /* We limit the number of times that we pass through vport_send() to
- * avoid blowing out the stack in the event that we have a loop. There is
- * a separate counter for each CPU for both interrupt and non-interrupt
+ * avoid blowing out the stack in the event that we have a loop. */
+struct loop_counter {
+	int count;		/* Current count. */
+	int max_count;		/* Highest count seen for current packet. */
+};
+
+/* We use a separate counter for each CPU for both interrupt and non-interrupt
  * context in order to keep the limit deterministic for a given packet. */
-struct percpu_loop_counter {
-	int count[2];
+struct percpu_loop_counters {
+	struct loop_counter counters[2];
 };
 
-static DEFINE_PER_CPU(struct percpu_loop_counter, vport_loop_counter);
+static DEFINE_PER_CPU(struct percpu_loop_counters, vport_loop_counters);
 #define VPORT_MAX_LOOPS 5
 
 /* Both RTNL lock and vport_mutex need to be held when updating dev_table.
@@ -1244,14 +1249,15 @@ static inline unsigned packet_length(const struct sk_buff *skb)
  */
 int vport_send(struct vport *vport, struct sk_buff *skb)
 {
-	int *loop_count;
+	struct loop_counter *loop;
 	int retval;
 	int mtu;
 
-	loop_count = &get_cpu_var(vport_loop_counter).count[!!in_interrupt()];
-	(*loop_count)++;
+	loop = &get_cpu_var(vport_loop_counters).counters[!!in_interrupt()];
+	if (++loop->count > loop->max_count)
+		loop->max_count = loop->count;
 
-	if (unlikely(*loop_count > VPORT_MAX_LOOPS)) {
+	if (unlikely(loop->max_count > VPORT_MAX_LOOPS)) {
 		if (net_ratelimit())
 			printk(KERN_WARNING "%s: dropping packet that has looped more than %d times\n",
 			       dp_name(vport_get_dp_port(vport)->dp), VPORT_MAX_LOOPS);
@@ -1288,8 +1294,9 @@ error:
 	kfree_skb(skb);
 	vport_record_error(vport, VPORT_E_TX_DROPPED);
 out:
-	(*loop_count)--;
-	put_cpu_var(vport_loop_counter);
+	if (!--loop->count)
+		loop->max_count = 0;
+	put_cpu_var(vport_loop_counters);
 
 	return retval;
 }
-- 
1.7.1





More information about the dev mailing list