datapath: Drop parameters from execute_actions().

It's (almost) always easier to understand a function with fewer parameters,
so this removes the now-redundant sw_flow_key and actions parameters from
execute_actions(), since they can be found through OVS_CB(skb)->flow now.

This also necessarily moves loop detection into execute_actions().
Otherwise, the flow's actions could have changed between the time that the
loop was detected and the time that it was suppressed, which would mean
that the wrong (version of the) flow would get suppressed.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>

diff --git a/datapath/actions.c b/datapath/actions.c
index a2a572e3f7df8974c6e3dc486b291d7b5f9d7b6d..b6b713534565bc6961c514a324df56dfebb0990d 100644 (file)
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -23,13 +23,13 @@
 #include "actions.h"
 #include "checksum.h"
 #include "datapath.h"
+#include "loop_counter.h"
 #include "openvswitch/datapath-protocol.h"
 #include "vlan.h"
 #include "vport.h"
 
 static int do_execute_actions(struct datapath *, struct sk_buff *,
-                             const struct sw_flow_key *,
-                             const struct nlattr *actions, u32 actions_len);
+                             struct sw_flow_actions *acts);
 
 static struct sk_buff *make_writable(struct sk_buff *skb, unsigned min_headroom)
 {
@@ -112,35 +112,34 @@ static struct sk_buff *modify_vlan_tci(struct sk_buff *skb, __be16 tci)
        return skb;
 }
 
-static bool is_ip(struct sk_buff *skb, const struct sw_flow_key *key)
+static bool is_ip(struct sk_buff *skb)
 {
-       return (key->dl_type == htons(ETH_P_IP) &&
+       return (OVS_CB(skb)->flow->key.dl_type == htons(ETH_P_IP) &&
                skb->transport_header > skb->network_header);
 }
 
-static __sum16 *get_l4_checksum(struct sk_buff *skb, const struct sw_flow_key *key)
+static __sum16 *get_l4_checksum(struct sk_buff *skb)
 {
+       u8 nw_proto = OVS_CB(skb)->flow->key.nw_proto;
        int transport_len = skb->len - skb_transport_offset(skb);
-       if (key->nw_proto == IPPROTO_TCP) {
+       if (nw_proto == IPPROTO_TCP) {
                if (likely(transport_len >= sizeof(struct tcphdr)))
                        return &tcp_hdr(skb)->check;
-       } else if (key->nw_proto == IPPROTO_UDP) {
+       } else if (nw_proto == IPPROTO_UDP) {
                if (likely(transport_len >= sizeof(struct udphdr)))
                        return &udp_hdr(skb)->check;
        }
        return NULL;
 }
 
-static struct sk_buff *set_nw_addr(struct sk_buff *skb,
-                                  const struct sw_flow_key *key,
-                                  const struct nlattr *a)
+static struct sk_buff *set_nw_addr(struct sk_buff *skb, const struct nlattr *a)
 {
        __be32 new_nwaddr = nla_get_be32(a);
        struct iphdr *nh;
        __sum16 *check;
        __be32 *nwaddr;
 
-       if (unlikely(!is_ip(skb, key)))
+       if (unlikely(!is_ip(skb)))
                return skb;
 
        skb = make_writable(skb, 0);
@@ -150,7 +149,7 @@ static struct sk_buff *set_nw_addr(struct sk_buff *skb,
        nh = ip_hdr(skb);
        nwaddr = nla_type(a) == ODP_ACTION_ATTR_SET_NW_SRC ? &nh->saddr : &nh->daddr;
 
-       check = get_l4_checksum(skb, key);
+       check = get_l4_checksum(skb);
        if (likely(check))
                inet_proto_csum_replace4(check, skb, *nwaddr, new_nwaddr, 1);
        csum_replace4(&nh->check, *nwaddr, new_nwaddr);
@@ -162,11 +161,9 @@ static struct sk_buff *set_nw_addr(struct sk_buff *skb,
        return skb;
 }
 
-static struct sk_buff *set_nw_tos(struct sk_buff *skb,
-                                 const struct sw_flow_key *key,
-                                 u8 nw_tos)
+static struct sk_buff *set_nw_tos(struct sk_buff *skb, u8 nw_tos)
 {
-       if (unlikely(!is_ip(skb, key)))
+       if (unlikely(!is_ip(skb)))
                return skb;
 
        skb = make_writable(skb, 0);
@@ -185,15 +182,13 @@ static struct sk_buff *set_nw_tos(struct sk_buff *skb,
        return skb;
 }
 
-static struct sk_buff *set_tp_port(struct sk_buff *skb,
-                                  const struct sw_flow_key *key,
-                                  const struct nlattr *a)
+static struct sk_buff *set_tp_port(struct sk_buff *skb, const struct nlattr *a)
 {
        struct udphdr *th;
        __sum16 *check;
        __be16 *port;
 
-       if (unlikely(!is_ip(skb, key)))
+       if (unlikely(!is_ip(skb)))
                return skb;
 
        skb = make_writable(skb, 0);
@@ -201,7 +196,7 @@ static struct sk_buff *set_tp_port(struct sk_buff *skb,
                return NULL;
 
        /* Must follow make_writable() since that can move the skb data. */
-       check = get_l4_checksum(skb, key);
+       check = get_l4_checksum(skb);
        if (unlikely(!check))
                return skb;
 
@@ -226,17 +221,16 @@ static struct sk_buff *set_tp_port(struct sk_buff *skb,
  *
  * @skb: skbuff containing an Ethernet packet, with network header pointing
  * just past the Ethernet and optional 802.1Q header.
- * @key: flow key extracted from @skb by flow_extract()
  *
  * Returns true if @skb is an invalid Ethernet+IPv4 ARP packet: one with screwy
  * or truncated header fields or one whose inner and outer Ethernet address
  * differ.
  */
-static bool is_spoofed_arp(struct sk_buff *skb, const struct sw_flow_key *key)
+static bool is_spoofed_arp(struct sk_buff *skb)
 {
        struct arp_eth_header *arp;
 
-       if (key->dl_type != htons(ETH_P_ARP))
+       if (OVS_CB(skb)->flow->key.dl_type != htons(ETH_P_ARP))
                return false;
 
        if (skb_network_offset(skb) + sizeof(struct arp_eth_header) > skb->len)
@@ -268,8 +262,7 @@ error:
        kfree_skb(skb);
 }
 
-static int output_control(struct datapath *dp, struct sk_buff *skb, u64 arg,
-                         const struct sw_flow_key *key)
+static int output_control(struct datapath *dp, struct sk_buff *skb, u64 arg)
 {
        struct dp_upcall_info upcall;
 
@@ -278,7 +271,7 @@ static int output_control(struct datapath *dp, struct sk_buff *skb, u64 arg,
                return -ENOMEM;
 
        upcall.cmd = ODP_PACKET_CMD_ACTION;
-       upcall.key = key;
+       upcall.key = &OVS_CB(skb)->flow->key;
        upcall.userdata = arg;
        upcall.sample_pool = 0;
        upcall.actions = NULL;
@@ -288,8 +281,7 @@ static int output_control(struct datapath *dp, struct sk_buff *skb, u64 arg,
 
 /* Execute a list of actions against 'skb'. */
 static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
-                             const struct sw_flow_key *key,
-                             const struct nlattr *actions, u32 actions_len)
+                             struct sw_flow_actions *acts)
 {
        /* 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
@@ -300,7 +292,8 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
        const struct nlattr *a;
        int rem, err;
 
-       for (a = actions, rem = actions_len; rem > 0; a = nla_next(a, &rem)) {
+       for (a = acts->actions, rem = acts->actions_len; rem > 0;
+            a = nla_next(a, &rem)) {
                if (prev_port != -1) {
                        do_output(dp, skb_clone(skb, GFP_ATOMIC), prev_port);
                        prev_port = -1;
@@ -312,7 +305,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
                        break;
 
                case ODP_ACTION_ATTR_CONTROLLER:
-                       err = output_control(dp, skb, nla_get_u64(a), key);
+                       err = output_control(dp, skb, nla_get_u64(a));
                        if (err) {
                                kfree_skb(skb);
                                return err;
@@ -347,16 +340,16 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 
                case ODP_ACTION_ATTR_SET_NW_SRC:
                case ODP_ACTION_ATTR_SET_NW_DST:
-                       skb = set_nw_addr(skb, key, a);
+                       skb = set_nw_addr(skb, a);
                        break;
 
                case ODP_ACTION_ATTR_SET_NW_TOS:
-                       skb = set_nw_tos(skb, key, nla_get_u8(a));
+                       skb = set_nw_tos(skb, nla_get_u8(a));
                        break;
 
                case ODP_ACTION_ATTR_SET_TP_SRC:
                case ODP_ACTION_ATTR_SET_TP_DST:
-                       skb = set_tp_port(skb, key, a);
+                       skb = set_tp_port(skb, a);
                        break;
 
                case ODP_ACTION_ATTR_SET_PRIORITY:
@@ -368,7 +361,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
                        break;
 
                case ODP_ACTION_ATTR_DROP_SPOOFED_ARP:
-                       if (unlikely(is_spoofed_arp(skb, key)))
+                       if (unlikely(is_spoofed_arp(skb)))
                                goto exit;
                        break;
                }
@@ -384,8 +377,7 @@ exit:
 }
 
 static void sflow_sample(struct datapath *dp, struct sk_buff *skb,
-                        const struct sw_flow_key *key,
-                        const struct nlattr *a, u32 actions_len)
+                        struct sw_flow_actions *acts)
 {
        struct sk_buff *nskb;
        struct vport *p = OVS_CB(skb)->vport;
@@ -403,23 +395,46 @@ static void sflow_sample(struct datapath *dp, struct sk_buff *skb,
                return;
 
        upcall.cmd = ODP_PACKET_CMD_SAMPLE;
-       upcall.key = key;
+       upcall.key = &OVS_CB(skb)->flow->key;
        upcall.userdata = 0;
        upcall.sample_pool = atomic_read(&p->sflow_pool);
-       upcall.actions = a;
-       upcall.actions_len = actions_len;
+       upcall.actions = acts->actions;
+       upcall.actions_len = acts->actions_len;
        dp_upcall(dp, nskb, &upcall);
 }
 
 /* Execute a list of actions against 'skb'. */
-int execute_actions(struct datapath *dp, struct sk_buff *skb,
-                   const struct sw_flow_key *key,
-                   const struct nlattr *actions, u32 actions_len)
+int execute_actions(struct datapath *dp, struct sk_buff *skb)
 {
-       if (dp->sflow_probability)
-               sflow_sample(dp, skb, key, actions, actions_len);
+       struct sw_flow_actions *acts = rcu_dereference(OVS_CB(skb)->flow->sf_acts);
+       struct loop_counter *loop;
+       int error;
+
+       /* Check whether we've looped too much. */
+       loop = loop_get_counter();
+       if (unlikely(++loop->count > MAX_LOOPS))
+               loop->looping = true;
+       if (unlikely(loop->looping)) {
+               error = loop_suppress(dp, acts);
+               kfree_skb(skb);
+               goto out_loop;
+       }
 
+       /* Really execute actions. */
+       if (dp->sflow_probability)
+               sflow_sample(dp, skb, acts);
        OVS_CB(skb)->tun_id = 0;
+       error = do_execute_actions(dp, skb, acts);
+
+       /* Check whether sub-actions looped too much. */
+       if (unlikely(loop->looping))
+               error = loop_suppress(dp, acts);
+
+out_loop:
+       /* Decrement loop counter. */
+       if (!--loop->count)
+               loop->looping = false;
+       loop_put_counter();
 
-       return do_execute_actions(dp, skb, key, actions, actions_len);
+       return error;
 }

diff --git a/datapath/actions.h b/datapath/actions.h
index 3348ae2c2be6f339a356297be4b99de6af14c033..a832295ce8043fefcc936eff8f64b82fe535ec62 100644 (file)
--- a/datapath/actions.h
+++ b/datapath/actions.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010 Nicira Networks.
+ * Copyright (c) 2009, 2010, 2011 Nicira Networks.
  * Distributed under the terms of the GNU GPL version 2.
  *
  * Significant portions of this file may be copied from parts of the Linux
@@ -16,9 +16,7 @@ struct datapath;
 struct sk_buff;
 struct sw_flow_key;
 
-int execute_actions(struct datapath *dp, struct sk_buff *skb,
-                   const struct sw_flow_key *,
-                   const struct nlattr *, u32 actions_len);
+int execute_actions(struct datapath *dp, struct sk_buff *skb);
 
 static inline void skb_clear_rxhash(struct sk_buff *skb)
 {

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 46dc7379c4ee91d9e2f111af01a2c24ffbdb14e4..05875dc1875b12692fe1f4861b42a852ddd98092 100644 (file)
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -49,7 +49,6 @@
 #include "datapath.h"
 #include "actions.h"
 #include "flow.h"
-#include "loop_counter.h"
 #include "table.h"
 #include "vlan.h"
 #include "vport-internal_dev.h"
@@ -267,8 +266,6 @@ void dp_process_received_packet(struct vport *p, struct sk_buff *skb)
        struct datapath *dp = p->dp;
        struct dp_stats_percpu *stats;
        int stats_counter_off;
-       struct sw_flow_actions *acts;
-       struct loop_counter *loop;
        int error;
 
        OVS_CB(skb)->vport = p;
@@ -313,32 +310,7 @@ void dp_process_received_packet(struct vport *p, struct sk_buff *skb)
 
        stats_counter_off = offsetof(struct dp_stats_percpu, n_hit);
        flow_used(OVS_CB(skb)->flow, skb);
-
-       acts = rcu_dereference(OVS_CB(skb)->flow->sf_acts);
-
-       /* Check whether we've looped too much. */
-       loop = loop_get_counter();
-       if (unlikely(++loop->count > MAX_LOOPS))
-               loop->looping = true;
-       if (unlikely(loop->looping)) {
-               loop_suppress(dp, acts);
-               kfree_skb(skb);
-               goto out_loop;
-       }
-
-       /* Execute actions. */
-       execute_actions(dp, skb, &OVS_CB(skb)->flow->key, acts->actions,
-                       acts->actions_len);
-
-       /* Check whether sub-actions looped too much. */
-       if (unlikely(loop->looping))
-               loop_suppress(dp, acts);
-
-out_loop:
-       /* Decrement loop counter. */
-       if (!--loop->count)
-               loop->looping = false;
-       loop_put_counter();
+       execute_actions(dp, skb);
 
 out:
        /* Update datapath statistics. */
@@ -739,9 +711,7 @@ static int odp_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
        err = -ENODEV;
        if (!dp)
                goto err_unlock;
-       err = execute_actions(dp, packet, &flow->key,
-                             nla_data(a[ODP_PACKET_ATTR_ACTIONS]),
-                             nla_len(a[ODP_PACKET_ATTR_ACTIONS]));
+       err = execute_actions(dp, packet);
        rcu_read_unlock();
 
        flow_put(flow);

diff --git a/datapath/loop_counter.c b/datapath/loop_counter.c
index 491305d97e5f93090e2c50fb997e12a58d347321..3e1d890aafccb80b430a78e38f700b8f476c5467 100644 (file)
--- a/datapath/loop_counter.c
+++ b/datapath/loop_counter.c
@@ -1,6 +1,6 @@
 /*
  * Distributed under the terms of the GNU GPL version 2.
- * Copyright (c) 2010 Nicira Networks.
+ * Copyright (c) 2010, 2011 Nicira Networks.
  *
  * Significant portions of this file may be copied from parts of the Linux
  * kernel, by Linus Torvalds and others.
@@ -15,12 +15,13 @@
 
 #include "loop_counter.h"
 
-void loop_suppress(struct datapath *dp, struct sw_flow_actions *actions)
+int loop_suppress(struct datapath *dp, struct sw_flow_actions *actions)
 {
        if (net_ratelimit())
                pr_warn("%s: flow looped %d times, dropping\n",
                        dp_name(dp), MAX_LOOPS);
        actions->actions_len = 0;
+       return -ELOOP;
 }
 
 #ifndef CONFIG_PREEMPT_RT

diff --git a/datapath/loop_counter.h b/datapath/loop_counter.h
index e08afb1d209340d4cd98da8434b4a7e463fb1345..0d1fdf960f1218f02a0e167772ff318bd58e0ffb 100644 (file)
--- a/datapath/loop_counter.h
+++ b/datapath/loop_counter.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010 Nicira Networks.
+ * Copyright (c) 2010, 2011 Nicira Networks.
  * Distributed under the terms of the GNU GPL version 2.
  *
  * Significant portions of this file may be copied from parts of the Linux
@@ -23,6 +23,6 @@ struct loop_counter {
 
 struct loop_counter *loop_get_counter(void);
 void loop_put_counter(void);
-void loop_suppress(struct datapath *, struct sw_flow_actions *);
+int loop_suppress(struct datapath *, struct sw_flow_actions *);
 
 #endif /* loop_counter.h */