[ovs-dev] [PATCH] datapath: handle recirculation loop detection

[Andy Zhou]

Current datapath allows the same packet to be executed up to 5 times to
avoid blowing out the kernel stack. Recirculation is currently also
counted as one execution, but does not use same amount of stack compare
to other services, such as IPsec.

This patch introduces the concept of stack cost. Recirculation has a
stack cost of 1 while other services have stack cost of 4. Datapath
packet process accommodates packets that need both services and
recirculation as long as the total stack cost does not exceed 16.
Packets exceed stack cost of 16 will be treated as looped packets
and dropped.

The behavior of packets do not recirculate does not change.

Signed-off-by: Andy Zhou <azhou at nicira.com>
---
 datapath/actions.c  | 28 ++++++++++++++++++----------
 datapath/datapath.c |  9 +++++----
 datapath/datapath.h |  4 ++--
 3 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/datapath/actions.c b/datapath/actions.c
index 5871d82..5556a0c 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -536,7 +536,7 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
 	recirc_key.ovs_flow_hash = hash;
 	recirc_key.recirc_id = nla_get_u32(a);
 
-	ovs_dp_process_packet_with_key(skb, &recirc_key);
+	ovs_dp_process_packet_with_key(skb, &recirc_key, true);
 
 	return 0;
 }
@@ -631,11 +631,16 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 }
 
 /* We limit the number of times that we pass into execute_actions()
- * to avoid blowing out the stack in the event that we have a loop. */
-#define MAX_LOOPS 4
+ * to avoid blowing out the stack in the event that we have a loop.
+ *
+ * Each loop adds some (estimated) cost to the kernel stack.
+ * The loop terminates When max cost is reached. */
+#define MAX_STACK_COST 16
+#define RECIRC_STACK_COST 1
+#define DEFAULT_STACK_COST 4
 
 struct loop_counter {
-	u8 count;		/* Count. */
+	u8 stack_cost;		/* loop stack cost. */
 	bool looping;		/* Loop detected? */
 };
 
@@ -644,22 +649,24 @@ static DEFINE_PER_CPU(struct loop_counter, loop_counters);
 static int loop_suppress(struct datapath *dp, struct sw_flow_actions *actions)
 {
 	if (net_ratelimit())
-		pr_warn("%s: flow looped %d times, dropping\n",
-				ovs_dp_name(dp), MAX_LOOPS);
+		pr_warn("%s: flow looped detected, dropping\n",
+				ovs_dp_name(dp));
 	actions->actions_len = 0;
 	return -ELOOP;
 }
 
 /* Execute a list of actions against 'skb'. */
-int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb)
+int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb, bool recirc)
 {
 	struct sw_flow_actions *acts = rcu_dereference(OVS_CB(skb)->flow->sf_acts);
+	const u8 stack_cost = recirc ? RECIRC_STACK_COST : DEFAULT_STACK_COST;
 	struct loop_counter *loop;
 	int error;
 
 	/* Check whether we've looped too much. */
 	loop = &__get_cpu_var(loop_counters);
-	if (unlikely(++loop->count > MAX_LOOPS))
+	loop->stack_cost += stack_cost;
+	if (unlikely(loop->stack_cost > MAX_STACK_COST))
 		loop->looping = true;
 	if (unlikely(loop->looping)) {
 		error = loop_suppress(dp, acts);
@@ -676,8 +683,9 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb)
 		error = loop_suppress(dp, acts);
 
 out_loop:
-	/* Decrement loop counter. */
-	if (!--loop->count)
+	/* Decrement loop stack cost. */
+	loop->stack_cost -= stack_cost;
+	if (!loop->stack_cost)
 		loop->looping = false;
 
 	return error;
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 10706f5..0611dad 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -241,7 +241,8 @@ void ovs_dp_detach_port(struct vport *p)
 }
 
 void ovs_dp_process_packet_with_key(struct sk_buff *skb,
-		struct sw_flow_key *pkt_key)
+				    struct sw_flow_key *pkt_key,
+				    bool recirc)
 {
 	const struct vport *p = OVS_CB(skb)->input_vport;
 	struct datapath *dp = p->dp;
@@ -272,7 +273,7 @@ void ovs_dp_process_packet_with_key(struct sk_buff *skb,
 	OVS_CB(skb)->flow = flow;
 
 	ovs_flow_stats_update(OVS_CB(skb)->flow, pkt_key->tp.flags, skb);
-	ovs_execute_actions(dp, skb);
+	ovs_execute_actions(dp, skb, recirc);
 	stats_counter = &stats->n_hit;
 
 out:
@@ -298,7 +299,7 @@ void ovs_dp_process_received_packet(struct vport *p, struct sk_buff *skb)
 		return;
 	}
 
-	ovs_dp_process_packet_with_key(skb, &key);
+	ovs_dp_process_packet_with_key(skb, &key, false);
 }
 
 int ovs_dp_upcall(struct datapath *dp, struct sk_buff *skb,
@@ -601,7 +602,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
 	OVS_CB(packet)->input_vport = input_vport;
 
 	local_bh_disable();
-	err = ovs_execute_actions(dp, packet);
+	err = ovs_execute_actions(dp, packet, false);
 	local_bh_enable();
 	rcu_read_unlock();
 
diff --git a/datapath/datapath.h b/datapath/datapath.h
index 6ddf72e..a847bd9 100644
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -191,7 +191,7 @@ extern struct genl_multicast_group ovs_dp_vport_multicast_group;
 
 void ovs_dp_process_received_packet(struct vport *, struct sk_buff *);
 void ovs_dp_process_packet_with_key(struct sk_buff *,
-				    struct sw_flow_key *pkt_key);
+				    struct sw_flow_key *pkt_key, bool recirc);
 void ovs_dp_detach_port(struct vport *);
 int ovs_dp_upcall(struct datapath *, struct sk_buff *,
 		  const struct dp_upcall_info *);
@@ -200,7 +200,7 @@ const char *ovs_dp_name(const struct datapath *dp);
 struct sk_buff *ovs_vport_cmd_build_info(struct vport *, u32 portid, u32 seq,
 					 u8 cmd);
 
-int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb);
+int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb, bool recirc);
 void ovs_dp_notify_wq(struct work_struct *work);
 
 #define OVS_NLERR(fmt, ...)					\
-- 
1.9.1