Thanks. Pushed to master with suggested changes.
On Fri, Sep 5, 2014 at 4:23 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Fri, Sep 5, 2014 at 2:52 PM, 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 FIFO', is used to store either
>> recirc or sample actions encountered during execution of an action
>> list. Not executing recirc or sample action in place, but rather execute
>> them laster as 'deferred actions' avoids recursion.
>>
>> Deferred actions are only executed after all other actions has been
>> executed, including the ones triggered by loopback from the kernel
>> network stack.
>>
>> The size of the private FIFO, currently set to 20, limits the number
>> of total 'deferred actions' any one packet can accumulate.
>>
>> Signed-off-by: Andy Zhou <az...@nicira.com>
>>
> Thanks for fixing all issue.
> Acked-by: Pravin B Shelar <pshe...@nicira.com>
>
> I have couple of minor comments below.
>
>> ---
>> v8->v7:
>> Always free skb from execute_recirc()
>> Dynamically allocating per CPU action fifos
>>
>> v6->v7:
>> Always clone the packet key
>>
>> v5->v6:
>> Remove ovs_ prefix for internal symbols.
>> Remove actions.h
>> Rename ovs_exec_actions_count to exec_actions_limit
>> Rename ovs_process_deferred_packets() to
>> process_deferred_actions()
>>
>> v4->v5:
>> Reset fifo after processing deferred actions
>> move private data structures from actions.h to actions.c
>> remove action_fifo init functions, since default percpu data
>> will be zero.
>> ---
>> datapath/actions.c | 169
>> +++++++++++++++++++++++++++++++++++++++++++++++-----
>> datapath/datapath.c | 5 ++
>> datapath/datapath.h | 3 +
>> 3 files changed, 162 insertions(+), 15 deletions(-)
>>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index 0a22e55..95f1e51 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -40,12 +40,80 @@
>> #include "vlan.h"
>> #include "vport.h"
>>
>> +struct deferred_action {
>> + struct sk_buff *skb;
>> + const struct nlattr *actions;
>> +
>> + /* Store pkt_key clone when creating deferred action. */
>> + struct sw_flow_key pkt_key;
>> +};
>> +
>> +#define DEFERRED_ACTION_FIFO_SIZE 10
>> +struct action_fifo {
>> + int head;
>> + int tail;
>> + /* Deferred action fifo queue storage. */
>> + struct deferred_action fifo[DEFERRED_ACTION_FIFO_SIZE];
>> +};
>> +
>> +static struct action_fifo *action_fifos;
>> +#define EXEC_ACTIONS_LEVEL_LIMIT 4 /* limit used to detect packet
>> + looping by the network stack */
>> +static DEFINE_PER_CPU(int, exec_actions_level);
>> +
>> +static void action_fifo_init(struct action_fifo *fifo)
>> +{
>> + fifo->head = 0;
>> + fifo->tail = 0;
>> +}
>> +
>> +static bool action_fifo_is_empty(struct action_fifo *fifo)
>> +{
>> + return (fifo->head == fifo->tail);
>> +}
>> +
>> +static struct deferred_action *
>> +action_fifo_get(struct action_fifo *fifo)
>> +{
>> + if (action_fifo_is_empty(fifo))
>> + return NULL;
>> +
>> + return &fifo->fifo[fifo->tail++];
>> +}
>> +
>> +static struct deferred_action *
>> +action_fifo_put(struct action_fifo *fifo)
>> +{
>> + if (fifo->head >= DEFERRED_ACTION_FIFO_SIZE - 1)
>> + return NULL;
>> +
>> + return &fifo->fifo[fifo->head++];
>> +}
>> +
>> static void flow_key_clone(struct sk_buff *skb, struct sw_flow_key *new_key)
>> {
>> *new_key = *OVS_CB(skb)->pkt_key;
>> OVS_CB(skb)->pkt_key = new_key;
>> }
>>
>> +/* Return True iff fifo is not full */
> Typo.
>
>> +static bool add_deferred_actions(struct sk_buff *skb,
>> + const struct nlattr *attr)
>> +{
>> + struct action_fifo *fifo;
>> + struct deferred_action *da;
>> +
>> + fifo = this_cpu_ptr(action_fifos);
>> + da = action_fifo_put(fifo);
>> + if (da) {
>> + da->skb = skb;
>> + da->actions = attr;
>> + flow_key_clone(skb, &da->pkt_key);
>> + }
>> +
>> + return (da != NULL);
>> +}
>> +
>> static void flow_key_set_recirc_id(struct sk_buff *skb, u32 recirc_id)
>> {
>> OVS_CB(skb)->pkt_key->recirc_id = recirc_id;
>> @@ -689,7 +757,6 @@ static bool last_action(const struct nlattr *a, int rem)
>> static int sample(struct datapath *dp, struct sk_buff *skb,
>> const struct nlattr *attr)
>> {
>> - struct sw_flow_key sample_key;
>> const struct nlattr *acts_list = NULL;
>> const struct nlattr *a;
>> int rem;
>> @@ -728,10 +795,15 @@ static int sample(struct datapath *dp, struct sk_buff
>> *skb,
>> /* Skip the sample action when out of memory. */
>> return 0;
>>
>> - flow_key_clone(skb, &sample_key);
>> + if (!add_deferred_actions(skb, a)) {
>> + if (net_ratelimit())
>> + pr_warn("%s: deferred actions limit reached,
>> dropping sample action\n",
>> + ovs_dp_name(dp));
>>
>> - /* do_execute_actions() will consume the cloned skb. */
>> - return do_execute_actions(dp, skb, a, rem);
>> + kfree_skb(skb);
>> + }
>> +
>> + return 0;
>> }
>>
>> static void execute_hash(struct sk_buff *skb, const struct nlattr *attr)
>> @@ -750,7 +822,7 @@ static void execute_hash(struct sk_buff *skb, const
>> struct nlattr *attr)
>> }
>>
>> static int execute_set_action(struct sk_buff *skb,
>> - const struct nlattr *nested_attr)
>> + const struct nlattr *nested_attr)
>> {
>> int err = 0;
>>
>> @@ -801,12 +873,9 @@ 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;
>> -
>> if (!is_skb_flow_key_valid(skb)) {
>> int err;
>>
>> @@ -819,25 +888,31 @@ static int execute_recirc(struct datapath *dp, struct
>> sk_buff *skb,
>>
>> if (!last_action(a, rem)) {
>> /* Recirc action is the not the last action
>> - * of the action list. */
>> + * 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;
>> + }
>>
>> - flow_key_clone(skb, &recirc_key);
>> + if (add_deferred_actions(skb, NULL)) {
>> + flow_key_set_recirc_id(skb, nla_get_u32(a));
>> + } else {
>> + kfree_skb(skb);
>> +
>> + if (net_ratelimit())
>> + pr_warn("%s: deferred action limit reached, drop
>> recirc action\n",
>> + ovs_dp_name(dp));
>> }
>>
>> - flow_key_set_recirc_id(skb, nla_get_u32(a));
>> - ovs_dp_process_packet(skb);
>> 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)
>> + const struct nlattr *attr, int len)
>> {
>> /* Every output action needs a separate clone of 'skb', but the
>> common
>> * case is just a single output action, so that doing a clone and
>> @@ -924,8 +999,72 @@ static int do_execute_actions(struct datapath *dp,
>> struct sk_buff *skb,
>> return 0;
>> }
>>
>> +static void process_deferred_actions(struct datapath *dp)
>> +{
>> + struct action_fifo *fifo = this_cpu_ptr(action_fifos);
>> +
>> + /* Do not touch the FIFO in case there is no deferred actions. */
>> + if (action_fifo_is_empty(fifo))
>> + return;
>> +
>> + /* Finishing executing all deferred actions. */
>> + do {
>> + struct deferred_action *da = action_fifo_get(fifo);
>> + struct sk_buff *skb = da->skb;
>> + const struct nlattr *actions = da->actions;
>> +
>> + if (actions)
>> + do_execute_actions(dp, skb, actions,
>> + nla_len(actions));
>> + else
>> + ovs_dp_process_packet(skb);
>> + } while (!action_fifo_is_empty(fifo));
>> +
>> + /* Reset FIFO for the next packet. */
>> + action_fifo_init(fifo);
>> +}
>> +
>> /* Execute a list of actions against 'skb'. */
>> -int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb, struct
>> sw_flow_actions *acts)
>> +int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb,
>> + struct sw_flow_actions *acts)
>> +{
>> + int level = this_cpu_read(exec_actions_level);
>> + int err;
>> +
>> + if (unlikely(level >= EXEC_ACTIONS_LEVEL_LIMIT)) {
>> + if (net_ratelimit())
>> + pr_warn("%s: packet loop detected, dropping.\n",
>> + ovs_dp_name(dp));
>> +
>> + kfree_skb(skb);
>> + return -ELOOP;
>> + }
>> +
>> + this_cpu_inc(exec_actions_level);
>> +
>> + err = do_execute_actions(dp, skb, acts->actions, acts->actions_len);
>> +
>> + if (!level)
>> + process_deferred_actions(dp);
>> +
>> + this_cpu_dec(exec_actions_level);
>> +
>> + /* This return status currently does not reflect the errors
>> + * encounted during deferred actions execution. Probably needs to
>> + * be fixed in the future. */
>> + return err;
>> +}
>> +
>> +int action_fifos_init(void)
>> +{
>> + action_fifos = alloc_percpu(struct action_fifo);
>> + if (!action_fifos)
>> + return -ENOMEM;
>> +
>> + return 0;
>> +}
>> +
>> +void action_fifos_exit(void)
>> {
>> - return do_execute_actions(dp, skb, acts->actions, acts->actions_len);
>> + free_percpu(action_fifos);
>> }
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index a668222..33ff7bc 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -2131,6 +2131,10 @@ static int __init dp_init(void)
>> pr_info("Open vSwitch switching datapath %s, built "__DATE__"
>> "__TIME__"\n",
>> VERSION);
>>
>> + err = action_fifos_init();
>> + if (err)
>> + goto error;
>> +
>> err = ovs_flow_init();
>> if (err)
>> goto error;
> Needs to free percpu memory in case of error here.
>
>> @@ -2173,6 +2177,7 @@ static void dp_cleanup(void)
>> rcu_barrier();
>> ovs_vport_exit();
>> ovs_flow_exit();
>> + action_fifos_exit();
>> }
>>
>> module_init(dp_init);
>> diff --git a/datapath/datapath.h b/datapath/datapath.h
>> index eba2fc4..c5d3c86 100644
>> --- a/datapath/datapath.h
>> +++ b/datapath/datapath.h
>> @@ -201,6 +201,9 @@ int ovs_execute_actions(struct datapath *dp, struct
>> sk_buff *skb,
>> struct sw_flow_actions *acts);
>> void ovs_dp_notify_wq(struct work_struct *work);
>>
>> +int action_fifos_init(void);
>> +void action_fifos_exit(void);
>> +
>> #define OVS_NLERR(fmt, ...) \
>> do { \
>> if (net_ratelimit()) \
>> --
>> 1.9.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
