[ovs-dev] [PATCH] datapath: simplify sample action implementation

Andy Zhou azhou at nicira.com
Fri Aug 29 22:21:43 UTC 2014


Thanks for the review. Pushed.

On Fri, Aug 29, 2014 at 2:32 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> Can you add some commit msg.
>
> On Fri, Aug 29, 2014 at 1:22 PM, Andy Zhou <azhou at nicira.com> wrote:
>> Signed-off-by: Andy Zhou <azhou at nicira.com>
>> ---
>>  datapath/actions.c | 45 +++++++++++++++++++--------------------------
>>  1 file changed, 19 insertions(+), 26 deletions(-)
>>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index 7f25553..c8951f0 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -688,7 +688,6 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
>>         struct sw_flow_key sample_key;
>>         const struct nlattr *acts_list = NULL;
>>         const struct nlattr *a;
>> -       struct sk_buff *sample_skb;
>>         int rem;
>>
>>         for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
>> @@ -708,33 +707,27 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
>>         rem = nla_len(acts_list);
>>         a = nla_data(acts_list);
>>
>> -       /* Actions list is either empty or only contains a single user-space
>> -        * action, the latter being a special case as it is the only known
>> -        * usage of the sample action.
>> -        * In these special cases don't clone the skb as there are no
>> -        * side-effects in the nested actions.
>> -        * Otherwise, clone in case the nested actions have side effects. */
>> -       if (likely(rem == 0 ||
>> -                  (nla_type(a) == OVS_ACTION_ATTR_USERSPACE &&
>> -                   last_action(a, rem)))) {
>> -               sample_skb = skb;
>> -               skb_get(skb);
>> -       } else {
>> -               sample_skb = skb_clone(skb, GFP_ATOMIC);
>> -               if (!sample_skb)
>> -                       /* Skip the sample action when out of memory. */
>> -                       return 0;
>> +       /* Actions list is empty, do nothing */
>> +       if (unlikely(!rem))
>> +               return 0;
>>
>> -               flow_key_clone(skb, &sample_key);
>> -       }
>> +       /* The only known useage of sample action is having a single user-space
>> +        * action. Treat this as a special case.
> s/useage/usage
>
>> +        * The output_userspace() should clone the skb to be sent to the
>> +        * user space. This skb will be consumed by its caller. */
>> +       if (likely(nla_type(a) == OVS_ACTION_ATTR_USERSPACE &&
>> +                  last_action(a, rem)))
>> +               return output_userspace(dp, skb, a);
>> +
>> +       skb = skb_clone(skb, GFP_ATOMIC);
>> +       if (!skb)
>> +               /* Skip the sample action when out of memory. */
>> +               return 0;
>> +
>> +       flow_key_clone(skb, &sample_key);
>>
>> -       /* Note that do_execute_actions() never consumes skb.
>> -        * In the case where skb has been cloned above it is the clone that
>> -        * is consumed.  Otherwise the skb_get(skb) call prevents
>> -        * consumption by do_execute_actions(). Thus, it is safe to simply
>> -        * return the error code and let the caller (also
>> -        * do_execute_actions()) free skb on error. */
>> -       return do_execute_actions(dp, sample_skb, a, rem);
>> +       /* do_execute_actions() will consume the cloned skb. */
>> +       return do_execute_actions(dp, skb, a, rem);
>>  }
>>
> sample() looks much better now.
>
> Acked-by: Pravin B Shelar <pshelar at nicira.com>
>
>>  static void execute_hash(struct sk_buff *skb, const struct nlattr *attr)
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list