On Fri, Aug 29, 2014 at 1:41 PM, Andy Zhou <az...@nicira.com> wrote:
> On Thu, Aug 28, 2014 at 3:40 PM, Pravin Shelar <pshe...@nicira.com> wrote:
>> On Wed, Aug 27, 2014 at 4:13 AM, Andy Zhou <az...@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 <az...@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().
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
