Re: [net-next sample action optimization v2 4/4] Openvswitch: Refactor sample and recirc actions implementation

[Pravin Shelar]

On Tue, Mar 14, 2017 at 4:08 PM, Andy Zhou  wrote:
> Added execute_or_defer_actions() that both sample and recirc
> action's implementation can use.
>
> Signed-off-by: Andy Zhou 
> ---
>  net/openvswitch/actions.c | 96 
> +--
>  1 file changed, 59 insertions(+), 37 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 1638370..fd7d903 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
...

> @@ -1286,6 +1262,52 @@ static int do_execute_actions(struct datapath *dp, 
> struct sk_buff *skb,
> return 0;
>  }
>
> +static int execute_or_defer_actions(struct datapath *dp, struct sk_buff *skb,
> +   struct sw_flow_key *clone,
> +   struct sw_flow_key *key,
> +   u32 *recirc_id,
> +   const struct nlattr *actions, int len)
> +{
> +   struct deferred_action *da;
> +
> +   /* If within the limit of 'OVS_DEFERRED_ACTION_THRESHOLD',
> +* recirc immediately, otherwise, defer it for later execution.
> +*/
> +   if (clone) {
> +   if (recirc_id) {
> +   clone->recirc_id = *recirc_id;
> +   ovs_dp_process_packet(skb, clone);
> +   return 0;
> +   } else {
> +   return do_execute_actions(dp, skb, clone,
> + actions, len);

the exec_actions_level inc and dec should be moved here, since it is
only needed around this function.

Re: [net-next sample action optimization v2 4/4] Openvswitch: Refactor sample and recirc actions implementation

[Pravin Shelar]

On Tue, Mar 14, 2017 at 4:08 PM, Andy Zhou  wrote:
> Added execute_or_defer_actions() that both sample and recirc
> action's implementation can use.
>
> Signed-off-by: Andy Zhou 
> ---
>  net/openvswitch/actions.c | 96 
> +--
>  1 file changed, 59 insertions(+), 37 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 1638370..fd7d903 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
...
...
> @@ -1286,6 +1262,52 @@ static int do_execute_actions(struct datapath *dp, 
> struct sk_buff *skb,
> return 0;
>  }
>
> +static int execute_or_defer_actions(struct datapath *dp, struct sk_buff *skb,
> +   struct sw_flow_key *clone,
> +   struct sw_flow_key *key,
> +   u32 *recirc_id,
> +   const struct nlattr *actions, int len)
> +{

The action pointer can be checked if it is recirc or sample action. so
no need to pass pointer to recirc id.
we can pass the last and clone_key flags to this function to clone the
skb and key in this function itself. This would make code bit easy to
read.

[net-next sample action optimization v2 4/4] Openvswitch: Refactor sample and recirc actions implementation

[Andy Zhou]

Added execute_or_defer_actions() that both sample and recirc
action's implementation can use.

Signed-off-by: Andy Zhou 
---
 net/openvswitch/actions.c | 96 +--
 1 file changed, 59 insertions(+), 37 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 1638370..fd7d903 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -44,10 +44,6 @@
 #include "conntrack.h"
 #include "vport.h"
 
-static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
- struct sw_flow_key *key,
- const struct nlattr *attr, int len);
-
 struct deferred_action {
struct sk_buff *skb;
const struct nlattr *actions;
@@ -166,6 +162,12 @@ static bool is_flow_key_valid(const struct sw_flow_key 
*key)
return !(key->mac_proto & SW_FLOW_KEY_INVALID);
 }
 
+static int execute_or_defer_actions(struct datapath *dp, struct sk_buff *skb,
+   struct sw_flow_key *clone,
+   struct sw_flow_key *key,
+   u32 *recirc_id,
+   const struct nlattr *actions, int len);
+
 static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
 __be16 ethertype)
 {
@@ -941,7 +943,7 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
struct nlattr *sample_arg;
struct sw_flow_key *orig_key = key;
int rem = nla_len(attr);
-   int err = 0;
+   int err;
const struct sample_arg *arg;
 
/* The first action is always 'OVS_SAMPLE_ATTR_ARG'. */
@@ -979,15 +981,8 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
key = clone_key(key);
}
 
-   if (key) {
-   err = do_execute_actions(dp, skb, key, actions, rem);
-   } else if (!add_deferred_actions(skb, orig_key, actions, rem)) {
-
-   if (net_ratelimit())
-   pr_warn("%s: deferred action limit reached, drop sample 
action\n",
-   ovs_dp_name(dp));
-   kfree_skb(skb);
-   }
+   err = execute_or_defer_actions(dp, skb, key, orig_key, NULL,
+  actions, rem);
 
if (!arg->exec)
__this_cpu_dec(exec_actions_level);
@@ -1105,8 +1100,7 @@ static int execute_recirc(struct datapath *dp, struct 
sk_buff *skb,
  struct sw_flow_key *key,
  const struct nlattr *a, int rem)
 {
-   struct sw_flow_key *recirc_key;
-   struct deferred_action *da;
+   u32 recirc_id;
 
if (!is_flow_key_valid(key)) {
int err;
@@ -1130,27 +1124,9 @@ static int execute_recirc(struct datapath *dp, struct 
sk_buff *skb,
return 0;
}
 
-   /* If within the limit of 'OVS_DEFERRED_ACTION_THRESHOLD',
-* recirc immediately, otherwise, defer it for later execution.
-*/
-   recirc_key = clone_key(key);
-   if (recirc_key) {
-   recirc_key->recirc_id = nla_get_u32(a);
-   ovs_dp_process_packet(skb, recirc_key);
-   } else {
-   da = add_deferred_actions(skb, key, NULL, 0);
-   if (da) {
-   recirc_key = >pkt_key;
-   recirc_key->recirc_id = nla_get_u32(a);
-   } else {
-   /* Log an error in case action fifo is full.  */
-   kfree_skb(skb);
-   if (net_ratelimit())
-   pr_warn("%s: deferred action limit reached, 
drop recirc action\n",
-   ovs_dp_name(dp));
-   }
-   }
-   return 0;
+   recirc_id = nla_get_u32(a);
+   return execute_or_defer_actions(dp, skb, clone_key(key),
+   key, _id, NULL, 0);
 }
 
 /* Execute a list of actions against 'skb'. */
@@ -1286,6 +1262,52 @@ static int do_execute_actions(struct datapath *dp, 
struct sk_buff *skb,
return 0;
 }
 
+static int execute_or_defer_actions(struct datapath *dp, struct sk_buff *skb,
+   struct sw_flow_key *clone,
+   struct sw_flow_key *key,
+   u32 *recirc_id,
+   const struct nlattr *actions, int len)
+{
+   struct deferred_action *da;
+
+   /* If within the limit of 'OVS_DEFERRED_ACTION_THRESHOLD',
+* recirc immediately, otherwise, defer it for later execution.
+*/
+   if (clone) {
+   if (recirc_id) {
+   clone->recirc_id = *recirc_id;
+   ovs_dp_process_packet(skb, clone);
+   return 0;
+   } else {
+   return