[ovs-dev] [v8 2/2] datapath: Implement recirc action without recursion

Pravin Shelar pshelar at nicira.com
Fri Sep 5 23:23:53 UTC 2014


On Fri, Sep 5, 2014 at 2:52 PM, 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 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 <azhou at nicira.com>
>
Thanks for fixing all issue.
Acked-by: Pravin B Shelar <pshelar at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list