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

[Andy Zhou]

On Sat, Mar 18, 2017 at 12:22 PM, Pravin Shelar  wrote:
> On Thu, Mar 16, 2017 at 3:48 PM, Andy Zhou  wrote:
>> Added clone_execute() that both the sample and the recirc
>> action implementation can use.
>>
>> Signed-off-by: Andy Zhou 
>> ---
>>  net/openvswitch/actions.c | 175 
>> --
>>  1 file changed, 92 insertions(+), 83 deletions(-)
>>
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index 3529f7b..e38fa7f 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 clone_execute(struct datapath *dp, struct sk_buff *skb,
>> +struct sw_flow_key *key,
>> +u32 recirc_id,
>> +const struct nlattr *actions, int len,
>> +bool last, bool clone_flow_key);
>> +
> With this function the diff stat looks much better.
>
> ...
> ...
>> +/* Execute the actions on the clone of the packet. The effect of the
>> + * execution does not affect the original 'skb' nor the original 'key'.
>> + *
>> + * The execution may be deferred in case the actions can not be executed
>> + * immediately.
>> + */
>> +static int clone_execute(struct datapath *dp, struct sk_buff *skb,
>> +struct sw_flow_key *key, u32 recirc_id,
>> +const struct nlattr *actions, int len,
>> +bool last, bool clone_flow_key)
>> +{
>> +   bool is_sample = actions;
> Standard practice is use !! to convert pointer to boolean.
> I think this function does not need to know about sample action. So we
> can rename the boolean to have_actions or something similar.
O.K.  We can just check for actions pointer.

However, it will be obvious we are actually checking for sample or recirc
action. I will add some comments.

>
>> +   struct deferred_action *da;
>> +   struct sw_flow_key *clone;
>> +   int err = 0;
>> +
>> +   skb = last ? skb : skb_clone(skb, GFP_ATOMIC);
>> +   if (!skb) {
>> +   /* Out of memory, skip this action.
>> +*/
>> +   return 0;
>> +   }
>> +
>> +   /* In case the sample actions won't change the 'key',
>> +* current key can be used directly to execute sample actions.
>> +* Otherwise, allocate a new key from the
>> +* next recursion level of 'flow_keys'. If
>> +* successful, execute the sample actions without
>> +* deferring.
>> +*/
>> +   if (is_sample && clone_flow_key)
>> +   __this_cpu_inc(exec_actions_level);
>> +
> There is no need to increment actions level up here. it is only
> required for do_execute_actions(). ovs_dp_process_packet() already
> does it.

Right, that's why it is only done for 'is_sample'. There is a bug though,
the 'inc' needs to be done after clone.  I will rearrange the code to move 'inc'
only above the do_execute_actions().
>
>
>> +   clone = clone_flow_key ? clone_key(key) : key;
>> +   if (clone) {
>> +   if (is_sample) {
>> +   err = do_execute_actions(dp, skb, clone,
>> +actions, len);
>> +   } else {
>> +   clone->recirc_id = recirc_id;
>> +   ovs_dp_process_packet(skb, clone);
>> +   }
>> +   }
> wont this execute action twice, once here and again in deferred actions list?

Right, the return is missing for the if (clone) case.  I will post v4
soon. Thanks for the review
and comments.
>
>> +
>> +   if (is_sample && clone_flow_key)
>> +   __this_cpu_dec(exec_actions_level);
>> +
>> +   /* Out of 'flow_keys' space. Defer them */
>> +   da = add_deferred_actions(skb, key, actions, len);
>> +   if (da) {
>> +   if (!is_sample) {
>> +   key = >pkt_key;
>> +   key->recirc_id = recirc_id;
>> +   }
>> +   } else {
>> +   /* Drop the SKB and log an error. */
>> +   kfree_skb(skb);
>> +
>> +   if (net_ratelimit()) {
>> +   if (is_sample) {
>> +   pr_warn("%s: deferred action limit reached, 
>> drop sample action\n",
>> +   ovs_dp_name(dp));
>> +   } else {
>> +   

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

[Pravin Shelar]

On Thu, Mar 16, 2017 at 3:48 PM, Andy Zhou  wrote:
> Added clone_execute() that both the sample and the recirc
> action implementation can use.
>
> Signed-off-by: Andy Zhou 
> ---
>  net/openvswitch/actions.c | 175 
> --
>  1 file changed, 92 insertions(+), 83 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 3529f7b..e38fa7f 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 clone_execute(struct datapath *dp, struct sk_buff *skb,
> +struct sw_flow_key *key,
> +u32 recirc_id,
> +const struct nlattr *actions, int len,
> +bool last, bool clone_flow_key);
> +
With this function the diff stat looks much better.

...
...
> +/* Execute the actions on the clone of the packet. The effect of the
> + * execution does not affect the original 'skb' nor the original 'key'.
> + *
> + * The execution may be deferred in case the actions can not be executed
> + * immediately.
> + */
> +static int clone_execute(struct datapath *dp, struct sk_buff *skb,
> +struct sw_flow_key *key, u32 recirc_id,
> +const struct nlattr *actions, int len,
> +bool last, bool clone_flow_key)
> +{
> +   bool is_sample = actions;
Standard practice is use !! to convert pointer to boolean.
I think this function does not need to know about sample action. So we
can rename the boolean to have_actions or something similar.

> +   struct deferred_action *da;
> +   struct sw_flow_key *clone;
> +   int err = 0;
> +
> +   skb = last ? skb : skb_clone(skb, GFP_ATOMIC);
> +   if (!skb) {
> +   /* Out of memory, skip this action.
> +*/
> +   return 0;
> +   }
> +
> +   /* In case the sample actions won't change the 'key',
> +* current key can be used directly to execute sample actions.
> +* Otherwise, allocate a new key from the
> +* next recursion level of 'flow_keys'. If
> +* successful, execute the sample actions without
> +* deferring.
> +*/
> +   if (is_sample && clone_flow_key)
> +   __this_cpu_inc(exec_actions_level);
> +
There is no need to increment actions level up here. it is only
required for do_execute_actions(). ovs_dp_process_packet() already
does it.


> +   clone = clone_flow_key ? clone_key(key) : key;
> +   if (clone) {
> +   if (is_sample) {
> +   err = do_execute_actions(dp, skb, clone,
> +actions, len);
> +   } else {
> +   clone->recirc_id = recirc_id;
> +   ovs_dp_process_packet(skb, clone);
> +   }
> +   }
wont this execute action twice, once here and again in deferred actions list?

> +
> +   if (is_sample && clone_flow_key)
> +   __this_cpu_dec(exec_actions_level);
> +
> +   /* Out of 'flow_keys' space. Defer them */
> +   da = add_deferred_actions(skb, key, actions, len);
> +   if (da) {
> +   if (!is_sample) {
> +   key = >pkt_key;
> +   key->recirc_id = recirc_id;
> +   }
> +   } else {
> +   /* Drop the SKB and log an error. */
> +   kfree_skb(skb);
> +
> +   if (net_ratelimit()) {
> +   if (is_sample) {
> +   pr_warn("%s: deferred action limit reached, 
> drop sample action\n",
> +   ovs_dp_name(dp));
> +   } else {
> +   pr_warn("%s: deferred action limit reached, 
> drop recirc action\n",
> +   ovs_dp_name(dp));
> +   }
> +   }
> +   }
> +
> +   return err;
> +}
> +
>  static void process_deferred_actions(struct datapath *dp)
>  {
> struct action_fifo *fifo = this_cpu_ptr(action_fifos);
> --
> 1.8.3.1
>

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

[Andy Zhou]

Added clone_execute() that both the sample and the recirc
action implementation can use.

Signed-off-by: Andy Zhou 
---
 net/openvswitch/actions.c | 175 --
 1 file changed, 92 insertions(+), 83 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 3529f7b..e38fa7f 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 clone_execute(struct datapath *dp, struct sk_buff *skb,
+struct sw_flow_key *key,
+u32 recirc_id,
+const struct nlattr *actions, int len,
+bool last, bool clone_flow_key);
+
 static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
 __be16 ethertype)
 {
@@ -938,10 +940,9 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
 {
struct nlattr *actions;
struct nlattr *sample_arg;
-   struct sw_flow_key *orig_key = key;
int rem = nla_len(attr);
-   int err = 0;
const struct sample_arg *arg;
+   bool clone_flow_key;
 
/* The first action is always 'OVS_SAMPLE_ATTR_ARG'. */
sample_arg = nla_data(attr);
@@ -955,43 +956,9 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
return 0;
}
 
-   /* Unless the last action, sample works on the clone of SKB.  */
-   skb = last ? skb : skb_clone(skb, GFP_ATOMIC);
-   if (!skb) {
-   /* Out of memory, skip this sample action.
-*/
-   return 0;
-   }
-
-   /* In case the sample actions won't change 'key',
-* it can be used directly to execute sample actions.
-* Otherwise, allocate a new key from the
-* next recursion level of 'flow_keys'. If
-* successful, execute the sample actions without
-* deferring.
-*
-* Defer the sample actions if the recursion
-* limit has been reached.
-*/
-   if (!arg->exec) {
-   __this_cpu_inc(exec_actions_level);
-   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);
-   }
-
-   if (!arg->exec)
-   __this_cpu_dec(exec_actions_level);
-
-   return err;
+   clone_flow_key = !arg->exec;
+   return clone_execute(dp, skb, key, 0, actions, rem, last,
+clone_flow_key);
 }
 
 static void execute_hash(struct sk_buff *skb, struct sw_flow_key *key,
@@ -1102,10 +1069,9 @@ static int execute_masked_set_action(struct sk_buff *skb,
 
 static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
  struct sw_flow_key *key,
- const struct nlattr *a, int rem)
+ const struct nlattr *a, bool last)
 {
-   struct sw_flow_key *recirc_key;
-   struct deferred_action *da;
+   u32 recirc_id;
 
if (!is_flow_key_valid(key)) {
int err;
@@ -1116,40 +1082,8 @@ static int execute_recirc(struct datapath *dp, struct 
sk_buff *skb,
}
BUG_ON(!is_flow_key_valid(key));
 
-   if (!nla_is_last(a, rem)) {
-   /* Recirc action is the not the last action
-* of the action list, need to clone the skb.
-*/
-   skb = skb_clone(skb, GFP_ATOMIC);
-
-   /* Skip the recirc action when out of memory, but
-* continue on with the rest of the action list.
-*/
-   if (!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 =