On Wed, Sep 3, 2014 at 1:05 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>
>
> ---
> 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.
This looks pretty close.
> ---
> datapath/Modules.mk | 1 +
> datapath/actions.c | 175
> ++++++++++++++++++++++++++++++++++++++++++++++++----
> datapath/actions.h | 31 ++++++++++
> datapath/datapath.c | 1 +
> datapath/datapath.h | 4 +-
> 5 files changed, 197 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..6ad5bbe 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -39,6 +39,74 @@
> #include "mpls.h"
> #include "vlan.h"
> #include "vport.h"
> +#include "actions.h"
> +
> +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;
> +};
> +
> +#define OVS_DEFERRED_ACTION_FIFO_SIZE 20
> +struct ovs_action_fifo {
> + int head;
> + int tail;
> + /* Deferred action fifo queue storage. */
> + struct ovs_deferred_action fifo[OVS_DEFERRED_ACTION_FIFO_SIZE];
> +};
> +
> +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);
> +
need better name.
> +static inline void action_fifo_init(struct ovs_action_fifo *fifo)
> +{
> + fifo->head = 0;
> + fifo->tail = 0;
> +}
> +
> +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;
> +}
>
There is no need to inline any symbols in .c file, it hides compiler
warnings of unused symbols.
> static void flow_key_clone(struct sk_buff *skb, struct sw_flow_key *new_key)
> {
> @@ -689,9 +757,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 +796,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));
>
> - /* do_execute_actions() will consume the cloned skb. */
> - return do_execute_actions(dp, skb, a, rem);
> + kfree_skb(skb);
> + return 0;
> + }
> +
> + flow_key_clone(skb, &da->pkt_key);
> +
> + return 0;
> }
>
> static void execute_hash(struct sk_buff *skb, const struct nlattr *attr)
> @@ -750,7 +827,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 +878,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 +903,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 +1016,67 @@ static int do_execute_actions(struct datapath *dp,
> struct sk_buff *skb,
> return 0;
> }
>
> +static void ovs_process_deferred_packets(struct datapath *dp)
> +{
deferred_packets is bit confusing since it same packet. can we use
deferred_action for function name?
> + struct ovs_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 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);
> + } while (!action_fifo_is_empty(fifo));
> +
> + /* Reset FIFO for the next packet. */
> + action_fifo_init(fifo);
> +}
In OVS kernel module "ovs_" prefix is used for non static symbol, so
can you remove all ovs prefix?
> +
> +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;
> + }
> +
unlike annotations.
> + return false;
> +}
> +
> +
extra line.
> /* 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)
> {
> - return do_execute_actions(dp, skb, acts->actions, acts->actions_len);
> + 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);
> +
> + 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;
> }
> diff --git a/datapath/actions.h b/datapath/actions.h
> new file mode 100644
> index 0000000..17749c0
> --- /dev/null
> +++ b/datapath/actions.h
> @@ -0,0 +1,31 @@
> +/* 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"
> +
> +int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb,
> + struct sw_flow_actions *acts);
> +void ovs_process_action_fifo(void);
> +
I do not see this symbol used anywhere. In this case the header
declares ovs_execute_actions(), which was declared in datapath.h
May be we do not need new header file for single function declaration.
> +#endif /* actions.h */
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index a668222..564bb71 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;
>
> 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, ...) \
> --
> 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
