Re: [ovs-dev] [PATCH] datapath: avoid deferred execution of recirc actions

[pravin shelar]

On Tue, Sep 20, 2016 at 5:55 AM, Lance Richardson  wrote:
> Port upstream fix to datapath module. The only notable difference
> between this patch and the upstream version is that the value of
> ovs_recursion_limit (5 for upstream kernel, 4 for out-of-tree
> module) is maintained in this patch.
>
> Upstream commit:
> commit f43e6dfb056b58628e43179d8f6b59eae417754d
> Author: Lance Richardson 
> Date:   Mon Sep 12 17:07:23 2016 -0400
>
> openvswitch: avoid deferred execution of recirc actions
>
> The ovs kernel data path currently defers the execution of all
> recirc actions until stack utilization is at a minimum.
> This is too limiting for some packet forwarding scenarios due to
> the small size of the deferred action FIFO (10 entries). For
> example, broadcast traffic sent out more than 10 ports with
> recirculation results in packet drops when the deferred action
> FIFO becomes full, as reported here:
>
>  http://openvswitch.org/pipermail/dev/2016-March/067672.html
>
> Since the current recursion depth is available (it is already tracked
> by the exec_actions_level pcpu variable), we can use it to determine
> whether to execute recirculation actions immediately (safe when
> recursion depth is low) or defer execution until more stack space is
> available.
>
> With this change, the deferred action fifo size becomes a non-issue
> for currently failing scenarios because it is no longer used when
> there are three or fewer recursions through ovs_execute_actions().
>
> Suggested-by: Pravin Shelar 
>
> Signed-off-by: Lance Richardson 

I pushed patch to master and branch-2.6.

Thanks.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] datapath: avoid deferred execution of recirc actions

[Lance Richardson]

Port upstream fix to datapath module. The only notable difference
between this patch and the upstream version is that the value of
ovs_recursion_limit (5 for upstream kernel, 4 for out-of-tree
module) is maintained in this patch.

Upstream commit:
commit f43e6dfb056b58628e43179d8f6b59eae417754d
Author: Lance Richardson 
Date:   Mon Sep 12 17:07:23 2016 -0400

openvswitch: avoid deferred execution of recirc actions

The ovs kernel data path currently defers the execution of all
recirc actions until stack utilization is at a minimum.
This is too limiting for some packet forwarding scenarios due to
the small size of the deferred action FIFO (10 entries). For
example, broadcast traffic sent out more than 10 ports with
recirculation results in packet drops when the deferred action
FIFO becomes full, as reported here:

 http://openvswitch.org/pipermail/dev/2016-March/067672.html

Since the current recursion depth is available (it is already tracked
by the exec_actions_level pcpu variable), we can use it to determine
whether to execute recirculation actions immediately (safe when
recursion depth is low) or defer execution until more stack space is
available.

With this change, the deferred action fifo size becomes a non-issue
for currently failing scenarios because it is no longer used when
there are three or fewer recursions through ovs_execute_actions().

Suggested-by: Pravin Shelar 

Signed-off-by: Lance Richardson 
---
 datapath/actions.c | 31 ---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/datapath/actions.c b/datapath/actions.c
index 3f1fb91..82833d0 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -70,6 +70,8 @@ struct ovs_frag_data {
 static DEFINE_PER_CPU(struct ovs_frag_data, ovs_frag_data_storage);
 
 #define DEFERRED_ACTION_FIFO_SIZE 10
+#define OVS_RECURSION_LIMIT 4
+#define OVS_DEFERRED_ACTION_THRESHOLD (OVS_RECURSION_LIMIT - 2)
 struct action_fifo {
int head;
int tail;
@@ -77,8 +79,12 @@ struct action_fifo {
struct deferred_action fifo[DEFERRED_ACTION_FIFO_SIZE];
 };
 
-static struct action_fifo __percpu *action_fifos;
+struct recirc_keys {
+   struct sw_flow_key key[OVS_DEFERRED_ACTION_THRESHOLD];
+};
 
+static struct action_fifo __percpu *action_fifos;
+static struct recirc_keys __percpu *recirc_keys;
 static DEFINE_PER_CPU(int, exec_actions_level);
 
 static void action_fifo_init(struct action_fifo *fifo)
@@ -1017,6 +1023,7 @@ static int execute_recirc(struct datapath *dp, struct 
sk_buff *skb,
  const struct nlattr *a, int rem)
 {
struct deferred_action *da;
+   int level;
 
if (!is_flow_key_valid(key)) {
int err;
@@ -1040,6 +1047,18 @@ static int execute_recirc(struct datapath *dp, struct 
sk_buff *skb,
return 0;
}
 
+   level = this_cpu_read(exec_actions_level);
+   if (level <= OVS_DEFERRED_ACTION_THRESHOLD) {
+   struct recirc_keys *rks = this_cpu_ptr(recirc_keys);
+   struct sw_flow_key *recirc_key = >key[level - 1];
+
+   *recirc_key = *key;
+   recirc_key->recirc_id = nla_get_u32(a);
+   ovs_dp_process_packet(skb, recirc_key);
+
+   return 0;
+   }
+
da = add_deferred_actions(skb, key, NULL);
if (da) {
da->pkt_key.recirc_id = nla_get_u32(a);
@@ -1206,11 +1225,10 @@ int ovs_execute_actions(struct datapath *dp, struct 
sk_buff *skb,
const struct sw_flow_actions *acts,
struct sw_flow_key *key)
 {
-   static const int ovs_recursion_limit = 4;
int err, level;
 
level = __this_cpu_inc_return(exec_actions_level);
-   if (unlikely(level > ovs_recursion_limit)) {
+   if (unlikely(level > OVS_RECURSION_LIMIT)) {
net_crit_ratelimited("ovs: recursion limit reached on datapath 
%s, probable configuration error\n",
 ovs_dp_name(dp));
kfree_skb(skb);
@@ -1235,10 +1253,17 @@ int action_fifos_init(void)
if (!action_fifos)
return -ENOMEM;
 
+   recirc_keys = alloc_percpu(struct recirc_keys);
+   if (!recirc_keys) {
+   free_percpu(action_fifos);
+   return -ENOMEM;
+   }
+
return 0;
 }
 
 void action_fifos_exit(void)
 {
free_percpu(action_fifos);
+   free_percpu(recirc_keys);
 }
-- 
2.5.5

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev