[ovs-dev] [dp-cleanups 4/4] datapath: Drop parameters from execute_actions().
Ben Pfaff
blp at nicira.com
Fri Apr 29 17:07:25 UTC 2011
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 at nicira.com>
---
datapath/actions.c | 107 +++++++++++++++++++++++++++--------------------
datapath/actions.h | 6 +--
datapath/datapath.c | 31 +-------------
datapath/loop_counter.c | 5 +-
datapath/loop_counter.h | 4 +-
5 files changed, 70 insertions(+), 83 deletions(-)
diff --git a/datapath/actions.c b/datapath/actions.c
index a2a572e..b6b7135 100644
--- 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 3348ae2..a832295 100644
--- 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 46dc737..8d96886 100644
--- 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;
@@ -314,31 +311,9 @@ 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 +714,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 491305d..3e1d890 100644
--- 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 e08afb1..0d1fdf9 100644
--- 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 */
--
1.7.4.4
More information about the dev
mailing list