[ovs-dev] [v3 3/4] datapath: Implement recirc action without recursion

Pravin Shelar pshelar at nicira.com
Fri Aug 29 22:20:53 UTC 2014


On Fri, Aug 29, 2014 at 1:41 PM, Andy Zhou <azhou at nicira.com> wrote:
> On Thu, Aug 28, 2014 at 3:40 PM, Pravin Shelar <pshelar at nicira.com> wrote:
>> On Wed, Aug 27, 2014 at 4:13 AM, Andy Zhou <azhou at nicira.com> wrote:
>>> Since kernel stack is limited in size, it is not wise to using
>>> recursive function with large stack frames.
>>>
>>> This patch provides an alternative implementation of recirc action
>>> without using recursion.
>>>
>>> A per CPU fixed sized, 'deferred action stack' is used to store both
>>> recirc and sample actions when executing actions. These actions becomes
>>> 'deferred actions".
>>>
>>> Deferred actions are only executed after all other actions has been
>>> executed, including the ones triggered by loopback from the kernel
>>> network stack.
>>>
>>> The depth of the private stack, currently set to 10, limits the number
>>> of 'deferred actions' can be accumulated for each packet.
>>>
>>> Signed-off-by: Andy Zhou <azhou at nicira.com>
>>
>> Patch looks good, I have few comments.
>>
>>> ---
>>>  datapath/actions.c  | 150 +++++++++++++++++++++++++++++++++++++---------------
>>>  datapath/actions.h  |   4 +-
>>>  datapath/datapath.c |   4 +-
>>>  3 files changed, 112 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/datapath/actions.c b/datapath/actions.c
>>> index 31fb57d..662de9c 100644
>>> --- a/datapath/actions.c
>>> +++ b/datapath/actions.c
>>> @@ -42,6 +42,7 @@
>>>  #include "actions.h"
>>>
>>>  static DEFINE_PER_CPU(struct ovs_action_stack, action_stacks);
>>> +DEFINE_PER_CPU(int, ovs_exec_actions_count);
>>>
>> ....
>>
>> @@ -741,33 +758,39 @@ static int sample(struct datapath *dp, struct
>> sk_buff *skb,
>>>         rem = nla_len(acts_list);
>>>         a = nla_data(acts_list);
>>>
>>> +       if (unlikely(rem == 0))
>>> +               return 0;
>>> +
>>>         /* 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);
>>> +       if (nla_type(a) == OVS_ACTION_ATTR_USERSPACE &&
>>> +           last_action(a, rem)) {
>>> +               return output_userspace(dp, skb, a);
>> This is optimization, should be kept in separate patch.
>>
>>>         } else {
>>> -               sample_skb = skb_clone(skb, GFP_ATOMIC);
>>> -               if (!sample_skb)
>>> -                       /* Skip the sample action when out of memory. */
>>> +               struct ovs_deferred_action *da;
>>> +
>>> +               skb = skb_clone(skb, GFP_ATOMIC);
>>> +               if (!skb)
>>> +                       /* Out of memory, skip the sample action. */
>>> +                       return 0;
>>> +
>>> +               da = push_actions(skb, a, rem);
>>> +               if (!da) {
>>> +                       if (net_ratelimit())
>>> +                               pr_warn("%s: stack limit reached, drop sample action\n",
>>> +                                       ovs_dp_name(dp));
>>> +                       kfree_skb(skb);
>>>                         return 0;
>>> +               }
>>>
>>> -               flow_key_clone(skb, &sample_key);
>>> +               flow_key_clone(skb, &da->pkt_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);
>>> +       return 0;
>>>  }
>>>
>>>  static void execute_hash(struct sk_buff *skb, const struct nlattr *attr)
>>> @@ -837,11 +860,10 @@ static int execute_set_action(struct sk_buff *skb,
>>>         return err;
>>>  }
>>>
>>> -
>>>  static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>>>                           const struct nlattr *a, int rem)
>>>  {
>>> -       struct sw_flow_key recirc_key;
>>> +       struct ovs_deferred_action *da;
>>>
>>>         if (!is_skb_flow_key_valid(skb)) {
>>>                 int err;
>>> @@ -854,26 +876,40 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>>>         BUG_ON(!is_skb_flow_key_valid(skb));
>>>
>>>         if (!last_action(a, rem)) {
>>> -               /* Recirc action is the not the last action
>>> -                * of the action list. */
>>> -               skb = skb_clone(skb, GFP_ATOMIC);
>>> +               /* Recirc action is the not the last action,
>>> +                * We will need to push skb clone instead of skb. */
>>>
>>> -               /* Skip the recirc action when out of memory, but
>>> -                * continue on with the rest of the action list. */
>>> +               skb = skb_clone(skb, GFP_ATOMIC);
>>>                 if (!skb)
>>> +                       /* Out of memory, skip recirc. */
>>>                         return 0;
>>>
>>> -               flow_key_clone(skb, &recirc_key);
>>> +               /* Push the skb clone for recirc. */
>>> +               da = push_actions(skb, NULL, 0);
>>> +               if (!da)
>>> +                       goto stack_full;
>>> +
>>> +               flow_key_clone(skb, &da->pkt_key);
>>> +       } else {
>>> +               skb_get(skb);
>> why do we need skb_get()?
>>
>>> +               if (!push_actions(skb, NULL, 0))
>>> +                       goto stack_full;
>>
>>>         }
>>>
>>>         flow_key_set_recirc_id(skb, nla_get_u32(a));
>>> -       ovs_dp_process_packet(skb);
>>> +       return 0;
>>> +
>>> +stack_full:
>>> +       if (net_ratelimit())
>>> +               pr_warn("%s: stack limit reached, drop recirc action\n",
>>> +                       ovs_dp_name(dp));
>>> +
>>>         return 0;
>>>  }
>>>
>>>  /* Execute a list of actions against 'skb'. */
>>> -static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>> -                       const struct nlattr *attr, int len)
>>> +static void do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>> +                              const struct nlattr *attr, int len)
>>>  {
>> what is reason for changing function prototype?
> Because the return value is not being used.  It is also not clear what
> the return code means.

Return code is used by ovs_packet_cmd_execute().



More information about the dev mailing list