On Fri, Aug 29, 2014 at 10:34 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>
> ---
> datapath/Modules.mk | 1 +
> datapath/actions.c | 178
> ++++++++++++++++++++++++++++++++++++++++++++++++----
> datapath/actions.h | 50 +++++++++++++++
> datapath/datapath.c | 4 ++
> datapath/datapath.h | 4 +-
> 5 files changed, 222 insertions(+), 15 deletions(-)
> create mode 100644 datapath/actions.h
>
> diff --git a/datapath/Modules.mk b/datapath/Modules.mk
> index 90e158c..2e74f6e 100644
> --- a/datapath/Modules.mk
> +++ b/datapath/Modules.mk
> @@ -23,6 +23,7 @@ openvswitch_sources = \
>
> openvswitch_headers = \
> compat.h \
> + actions.h \
> datapath.h \
> flow.h \
> flow_netlink.h \
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 0a22e55..0dcf1a8 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -39,6 +39,58 @@
> #include "mpls.h"
> #include "vlan.h"
> #include "vport.h"
> +#include "actions.h"
> +
> +static DEFINE_PER_CPU(struct ovs_action_fifo, action_fifos);
> +#define OVS_EXEC_ACTIONS_COUNT_LIMIT 4 /* limit used to detect packet
> + looping by the network stack */
> +static DEFINE_PER_CPU(int, ovs_exec_actions_count);
> +
Count is not very clear, can you use other name? something like
recursion, loop or depth?
> +static inline void action_fifo_init(struct ovs_action_fifo *fifo)
> +{
> + fifo->head = 0;
> + fifo->tail = 0;
> +}
> +
static memory should be zero, so no need to initialize it.
> +static inline bool action_fifo_is_empty(struct ovs_action_fifo *fifo)
> +{
> + return (fifo->head == fifo->tail);
> +}
> +
> +static inline struct ovs_deferred_action *
> +action_fifo_get(struct ovs_action_fifo *fifo)
> +{
> + if (action_fifo_is_empty(fifo))
> + return NULL;
> +
> + return &fifo->fifo[fifo->tail++];
> +}
> +
> +static inline struct ovs_deferred_action *
> +action_fifo_put(struct ovs_action_fifo *fifo)
> +{
> + if (fifo->head >= OVS_DEFERRED_ACTION_FIFO_SIZE - 1)
> + return NULL;
> +
> + return &fifo->fifo[fifo->head++];
> +}
> +
> +static inline struct ovs_deferred_action *
> +add_deferred_actions(struct sk_buff *skb, const struct nlattr *attr)
> +{
> + struct ovs_action_fifo *fifo;
> + struct ovs_deferred_action *da;
> +
> + fifo = this_cpu_ptr(&(action_fifos));
> + da = action_fifo_put(fifo);
> +
> + if (da) {
> + da->skb = skb;
> + da->actions = attr;
> + }
> +
> + return da;
> +}
>
> static void flow_key_clone(struct sk_buff *skb, struct sw_flow_key *new_key)
> {
> @@ -689,9 +741,9 @@ 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;
> + struct ovs_deferred_action *da;
> int rem;
>
> for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
> @@ -728,10 +780,19 @@ 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);
> + da = add_deferred_actions(skb, a);
> + if (!da) {
> + if (net_ratelimit())
> + pr_warn("%s: deferred actions limit reached, dropping
> sample action\n",
> + ovs_dp_name(dp));
> +
> + kfree_skb(skb);
> + return 0;
> + }
> +
> + flow_key_clone(skb, &da->pkt_key);
>
> - /* do_execute_actions() will consume the cloned skb. */
> - return do_execute_actions(dp, skb, a, rem);
> + return 0;
> }
>
> static void execute_hash(struct sk_buff *skb, const struct nlattr *attr)
> @@ -750,7 +811,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,11 +862,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;
> @@ -827,17 +887,33 @@ static int execute_recirc(struct datapath *dp, struct
> sk_buff *skb,
> if (!skb)
> return 0;
>
> - flow_key_clone(skb, &recirc_key);
> + /* Add the skb clone to action fifo. */
> + da = add_deferred_actions(skb, NULL);
> + if (!da) {
> + kfree_skb(skb);
> + goto fifo_full;
> + }
> +
> + flow_key_clone(skb, &da->pkt_key);
> + } else {
> + if (!add_deferred_actions(skb, NULL))
> + goto fifo_full;
> }
>
> flow_key_set_recirc_id(skb, nla_get_u32(a));
> - ovs_dp_process_packet(skb);
> + return 0;
> +
> +fifo_full:
> + if (net_ratelimit())
> + pr_warn("%s: deferred action 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)
> + 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 +1000,86 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
> return 0;
> }
>
> +static void ovs_process_deferred_packets(struct datapath *dp)
> +{
> + struct ovs_action_fifo *fifo;
> +
> + fifo = this_cpu_ptr(&(action_fifos));
> +
> + /* Finishing executing all deferred actions. */
> + while (!action_fifo_is_empty(fifo)) {
> + struct ovs_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);
> + }
need to reset head and tail indices, otherwise it will cause buffer over-run.
> +}
> +
> +static bool ovs_exec_actions_limit_exceeded(struct datapath *dp, int count)
> +{
> + if (count >= OVS_EXEC_ACTIONS_COUNT_LIMIT) {
> + if (net_ratelimit())
> + pr_warn("%s: packet loop detected, dropping.\n",
> + ovs_dp_name(dp));
> +
> + return true;
> + }
> +
> + return false;
> +}
> +
> +
> /* 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 count = this_cpu_read(ovs_exec_actions_count);
> + int err;
> +
> + if (ovs_exec_actions_limit_exceeded(dp, count)) {
> + kfree_skb(skb);
> + return -ELOOP;
> + }
> +
> + this_cpu_inc(ovs_exec_actions_count);
> +
> + err = do_execute_actions(dp, skb, acts->actions, acts->actions_len);
> +
> + if (!count)
> + ovs_process_deferred_packets(dp);
> +
Since count is decremented after the check, above check will never be true.
> + this_cpu_dec(ovs_exec_actions_count);
> +
> + /* This return status currently does not reflect the errors
> + * encounted during deferred actions execution. Probably needs to
> + * be fixed in the future. */
> + return err;
> +}
> +
> +void ovs_action_fifos_init(void)
> {
> - return do_execute_actions(dp, skb, acts->actions, acts->actions_len);
> + int i;
> +
> + for_each_possible_cpu(i) {
> + struct ovs_action_fifo *fifo;
> +
> + fifo = &per_cpu(action_fifos, i);
> + action_fifo_init(fifo);
> + }
> +}
> +
> +void ovs_action_fifos_exit(void)
> +{
> + int i;
> +
> + for_each_possible_cpu(i) {
> + struct ovs_action_fifo *fifo;
> +
> + fifo = &per_cpu(action_fifos, i);
> + BUG_ON(!action_fifo_is_empty(fifo));
> + }
> }
> diff --git a/datapath/actions.h b/datapath/actions.h
> new file mode 100644
> index 0000000..7db6a7f
> --- /dev/null
> +++ b/datapath/actions.h
> @@ -0,0 +1,50 @@
> +/* Copyright (c) 2014 Nicira, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#ifndef ACTIONS_H
> +#define ACTIONS_H 1
> +
> +#include <linux/kernel.h>
> +#include <linux/mutex.h>
> +#include <linux/netdevice.h>
> +#include <linux/skbuff.h>
> +
> +#include "compat.h"
> +#include "flow.h"
> +#include "flow_table.h"
> +#include "vlan.h"
> +#include "vport.h"
> +
> +#define OVS_DEFERRED_ACTION_FIFO_SIZE 20
> +
> +struct ovs_deferred_action {
> + struct sk_buff *skb;
> + const struct nlattr *actions;
> +
> + /* Store pkt_key clone when creating deferred action. */
> + struct sw_flow_key pkt_key;
> +};
> +
> +struct ovs_action_fifo {
> + int head;
> + int tail;
> + /* Deferred action fifo queue storage. */
> + struct ovs_deferred_action fifo[OVS_DEFERRED_ACTION_FIFO_SIZE];
> +};
> +
There is not need to expose these definition outside action.c, so let
move it there.
> +void ovs_action_fifos_init(void);
> +void ovs_action_fifos_exit(void);
> +int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb,
> + struct sw_flow_actions *acts);
> +void ovs_process_action_fifo(void);
> +
> +#endif /* actions.h */
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index a668222..9ec926d 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -61,6 +61,7 @@
> #include "vlan.h"
> #include "vport-internal_dev.h"
> #include "vport-netdev.h"
> +#include "actions.h"
>
> int ovs_net_id __read_mostly;
>
> @@ -2151,6 +2152,8 @@ static int __init dp_init(void)
> if (err < 0)
> goto error_unreg_notifier;
>
> + ovs_action_fifos_init();
> +
This needs to be at start so that we can execute actions on packet
received on datapath.
> return 0;
>
> error_unreg_notifier:
> @@ -2173,6 +2176,7 @@ static void dp_cleanup(void)
> rcu_barrier();
> ovs_vport_exit();
> ovs_flow_exit();
> + ovs_action_fifos_exit();
> }
>
> module_init(dp_init);
> diff --git a/datapath/datapath.h b/datapath/datapath.h
> index eba2fc4..dbe58d4 100644
> --- a/datapath/datapath.h
> +++ b/datapath/datapath.h
> @@ -31,6 +31,7 @@
> #include "flow_table.h"
> #include "vlan.h"
> #include "vport.h"
> +#include "actions.h"
>
> #define DP_MAX_PORTS USHRT_MAX
> #define DP_VPORT_HASH_BUCKETS 1024
> @@ -196,9 +197,6 @@ int ovs_dp_upcall(struct datapath *, struct sk_buff *,
> const char *ovs_dp_name(const struct datapath *dp);
> struct sk_buff *ovs_vport_cmd_build_info(struct vport *, u32 portid, u32 seq,
> u8 cmd);
> -
> -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);
>
> #define OVS_NLERR(fmt, ...) \
> --
Thanks.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
