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
