On Thu, Mar 16, 2017 at 3:48 PM, Andy Zhou <az...@ovn.org> wrote:
> Added clone_execute() that both the sample and the recirc
> action implementation can use.
>
> Signed-off-by: Andy Zhou <az...@ovn.org>
> ---
> net/openvswitch/actions.c | 175
> ++++++++++++++++++++++++----------------------
> 1 file changed, 92 insertions(+), 83 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 3529f7b..e38fa7f 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -44,10 +44,6 @@
> #include "conntrack.h"
> #include "vport.h"
>
> -static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> - struct sw_flow_key *key,
> - const struct nlattr *attr, int len);
> -
> struct deferred_action {
> struct sk_buff *skb;
> const struct nlattr *actions;
> @@ -166,6 +162,12 @@ static bool is_flow_key_valid(const struct sw_flow_key
> *key)
> return !(key->mac_proto & SW_FLOW_KEY_INVALID);
> }
>
> +static int clone_execute(struct datapath *dp, struct sk_buff *skb,
> + struct sw_flow_key *key,
> + u32 recirc_id,
> + const struct nlattr *actions, int len,
> + bool last, bool clone_flow_key);
> +
With this function the diff stat looks much better.
...
...
> +/* Execute the actions on the clone of the packet. The effect of the
> + * execution does not affect the original 'skb' nor the original 'key'.
> + *
> + * The execution may be deferred in case the actions can not be executed
> + * immediately.
> + */
> +static int clone_execute(struct datapath *dp, struct sk_buff *skb,
> + struct sw_flow_key *key, u32 recirc_id,
> + const struct nlattr *actions, int len,
> + bool last, bool clone_flow_key)
> +{
> + bool is_sample = actions;
Standard practice is use !! to convert pointer to boolean.
I think this function does not need to know about sample action. So we
can rename the boolean to have_actions or something similar.
> + struct deferred_action *da;
> + struct sw_flow_key *clone;
> + int err = 0;
> +
> + skb = last ? skb : skb_clone(skb, GFP_ATOMIC);
> + if (!skb) {
> + /* Out of memory, skip this action.
> + */
> + return 0;
> + }
> +
> + /* In case the sample actions won't change the 'key',
> + * current key can be used directly to execute sample actions.
> + * Otherwise, allocate a new key from the
> + * next recursion level of 'flow_keys'. If
> + * successful, execute the sample actions without
> + * deferring.
> + */
> + if (is_sample && clone_flow_key)
> + __this_cpu_inc(exec_actions_level);
> +
There is no need to increment actions level up here. it is only
required for do_execute_actions(). ovs_dp_process_packet() already
does it.
> + clone = clone_flow_key ? clone_key(key) : key;
> + if (clone) {
> + if (is_sample) {
> + err = do_execute_actions(dp, skb, clone,
> + actions, len);
> + } else {
> + clone->recirc_id = recirc_id;
> + ovs_dp_process_packet(skb, clone);
> + }
> + }
wont this execute action twice, once here and again in deferred actions list?
> +
> + if (is_sample && clone_flow_key)
> + __this_cpu_dec(exec_actions_level);
> +
> + /* Out of 'flow_keys' space. Defer them */
> + da = add_deferred_actions(skb, key, actions, len);
> + if (da) {
> + if (!is_sample) {
> + key = &da->pkt_key;
> + key->recirc_id = recirc_id;
> + }
> + } else {
> + /* Drop the SKB and log an error. */
> + kfree_skb(skb);
> +
> + if (net_ratelimit()) {
> + if (is_sample) {
> + pr_warn("%s: deferred action limit reached,
> drop sample action\n",
> + ovs_dp_name(dp));
> + } else {
> + pr_warn("%s: deferred action limit reached,
> drop recirc action\n",
> + ovs_dp_name(dp));
> + }
> + }
> + }
> +
> + return err;
> +}
> +
> static void process_deferred_actions(struct datapath *dp)
> {
> struct action_fifo *fifo = this_cpu_ptr(action_fifos);
> --
> 1.8.3.1
>
