From: Ben Pfaff Date: Thu, 24 Jul 2008 23:07:32 +0000 (-0700) Subject: Merge branch 'locking' X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=be25fa83e8f65d92b853bd20a3d1a594af352b92;hp=-c;p=openvswitch Merge branch 'locking' --- be25fa83e8f65d92b853bd20a3d1a594af352b92 diff --combined datapath/datapath.c index 147b7aa4,d55ac2ea..a303f500 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@@ -28,7 -28,6 +28,7 @@@ #include #include #include +#include #include "openflow-netlink.h" #include "datapath.h" @@@ -64,15 -63,16 +64,16 @@@ static struct genl_multicast_group mc_g /* It's hard to imagine wanting more than one datapath, but... */ #define DP_MAX 32 - /* datapaths. Protected on the read side by rcu_read_lock, on the write side - * by dp_mutex. + /* Datapaths. Protected on the read side by rcu_read_lock, on the write side + * by dp_mutex. dp_mutex is almost completely redundant with genl_mutex + * maintained by the Generic Netlink code, but the timeout path needs mutual + * exclusion too. * * It is safe to access the datapath and net_bridge_port structures with just - * the dp_mutex, but to access the chain you need to take the rcu_read_lock - * also (because dp_mutex doesn't prevent flows from being destroyed). + * dp_mutex. */ static struct datapath *dps[DP_MAX]; - static DEFINE_MUTEX(dp_mutex); + DEFINE_MUTEX(dp_mutex); static int dp_maint_func(void *data); static int send_port_status(struct net_bridge_port *p, uint8_t status); @@@ -242,9 -242,7 +243,7 @@@ uint64_t gen_datapath_id(uint16_t dp_id } /* Creates a new datapath numbered 'dp_idx'. Returns 0 for success or a - * negative error code. - * - * Not called with any locks. */ + * negative error code. */ static int new_dp(int dp_idx) { struct datapath *dp; @@@ -256,9 -254,8 +255,8 @@@ if (!try_module_get(THIS_MODULE)) return -ENODEV; - mutex_lock(&dp_mutex); - dp = rcu_dereference(dps[dp_idx]); - if (dp != NULL) { + /* Exit early if a datapath with that number already exists. */ + if (dps[dp_idx]) { err = -EEXIST; goto err_unlock; } @@@ -293,8 -290,7 +291,7 @@@ if (IS_ERR(dp->dp_task)) goto err_destroy_chain; - rcu_assign_pointer(dps[dp_idx], dp); - mutex_unlock(&dp_mutex); + dps[dp_idx] = dp; return 0; @@@ -307,12 -303,11 +304,11 @@@ err_destroy_dp_dev err_free_dp: kfree(dp); err_unlock: - mutex_unlock(&dp_mutex); module_put(THIS_MODULE); return err; } - /* Find and return a free port number under 'dp'. Called under dp_mutex. */ + /* Find and return a free port number under 'dp'. */ static int find_portno(struct datapath *dp) { int i; @@@ -350,7 -345,6 +346,6 @@@ static struct net_bridge_port *new_nbp( return p; } - /* Called with dp_mutex. */ int add_switch_port(struct datapath *dp, struct net_device *dev) { struct net_bridge_port *p; @@@ -374,8 -368,7 +369,7 @@@ return 0; } - /* Delete 'p' from switch. - * Called with dp_mutex. */ + /* Delete 'p' from switch. */ static int del_switch_port(struct net_bridge_port *p) { /* First drop references to device. */ @@@ -399,7 -392,6 +393,6 @@@ return 0; } - /* Called with dp_mutex. */ static void del_dp(struct datapath *dp) { struct net_bridge_port *p, *n; @@@ -467,8 -459,6 +460,6 @@@ static int dp_frame_hook(struct net_bri } #else /* NB: This has only been tested on 2.4.35 */ - - /* Called without any locks (?) */ static void dp_frame_hook(struct sk_buff *skb) { struct net_bridge_port *p = skb->dev->br_port; @@@ -882,7 -872,6 +873,6 @@@ static int dp_genl_del(struct sk_buff * if (!info->attrs[DP_GENL_A_DP_IDX]) return -EINVAL; - mutex_lock(&dp_mutex); dp = dp_get(nla_get_u32((info->attrs[DP_GENL_A_DP_IDX]))); if (!dp) err = -ENOENT; @@@ -890,7 -879,6 +880,6 @@@ del_dp(dp); err = 0; } - mutex_unlock(&dp_mutex); return err; } @@@ -974,7 -962,6 +963,6 @@@ static int dp_genl_add_del_port(struct return -EINVAL; /* Get datapath. */ - mutex_lock(&dp_mutex); dp = dp_get(nla_get_u32(info->attrs[DP_GENL_A_DP_IDX])); if (!dp) { err = -ENOENT; @@@ -1003,7 -990,6 +991,6 @@@ out_put: dev_put(port); out: - mutex_unlock(&dp_mutex); return err; } @@@ -1034,26 -1020,22 +1021,22 @@@ static int dp_genl_openflow(struct sk_b if (!info->attrs[DP_GENL_A_DP_IDX] || !va) return -EINVAL; - rcu_read_lock(); dp = dp_get(nla_get_u32(info->attrs[DP_GENL_A_DP_IDX])); - if (!dp) { - err = -ENOENT; - goto out; - } + if (!dp) + return -ENOENT; - if (nla_len(va) < sizeof(struct ofp_header)) { - err = -EINVAL; - goto out; - } + if (nla_len(va) < sizeof(struct ofp_header)) + return -EINVAL; oh = nla_data(va); sender.xid = oh->xid; sender.pid = info->snd_pid; sender.seq = info->snd_seq; - err = fwd_control_input(dp->chain, &sender, nla_data(va), nla_len(va)); - out: - rcu_read_unlock(); + mutex_lock(&dp_mutex); + err = fwd_control_input(dp->chain, &sender, + nla_data(va), nla_len(va)); + mutex_unlock(&dp_mutex); return err; } @@@ -1369,7 -1351,6 +1352,6 @@@ dp_genl_openflow_dumpit(struct sk_buff * struct genl_ops. This kluge supports earlier versions also. */ cb->done = dp_genl_openflow_done; - rcu_read_lock(); if (!cb->args[0]) { struct nlattr *attrs[DP_GENL_A_MAX + 1]; struct ofp_stats_request *rq; @@@ -1382,21 -1363,17 +1364,17 @@@ if (err < 0) return err; - err = -EINVAL; - if (!attrs[DP_GENL_A_DP_IDX]) - goto out; + return -EINVAL; dp_idx = nla_get_u16(attrs[DP_GENL_A_DP_IDX]); dp = dp_get(dp_idx); - if (!dp) { - err = -ENOENT; - goto out; - } + if (!dp) + return -ENOENT; va = attrs[DP_GENL_A_OPENFLOW]; len = nla_len(va); if (!va || len < sizeof *rq) - goto out; + return -EINVAL; rq = nla_data(va); type = ntohs(rq->type); @@@ -1405,12 -1382,12 +1383,12 @@@ || ntohs(rq->header.length) != len || type >= ARRAY_SIZE(stats) || !stats[type].dump) - goto out; + return -EINVAL; s = &stats[type]; body_len = len - offsetof(struct ofp_stats_request, body); if (body_len < s->min_body || body_len > s->max_body) - goto out; + return -EINVAL; cb->args[0] = 1; cb->args[1] = dp_idx; @@@ -1420,7 -1397,7 +1398,7 @@@ void *state; err = s->init(dp, rq->body, body_len, &state); if (err) - goto out; + return err; cb->args[4] = (long) state; } } else if (cb->args[0] == 1) { @@@ -1428,13 -1405,10 +1406,10 @@@ s = &stats[cb->args[2]]; dp = dp_get(dp_idx); - if (!dp) { - err = -ENOENT; - goto out; - } + if (!dp) + return -ENOENT; } else { - err = 0; - goto out; + return 0; } sender.xid = cb->args[3]; @@@ -1443,10 -1417,8 +1418,8 @@@ osr = put_openflow_headers(dp, skb, OFPT_STATS_REPLY, &sender, &max_openflow_len); - if (IS_ERR(osr)) { - err = PTR_ERR(osr); - goto out; - } + if (IS_ERR(osr)) + return PTR_ERR(osr); osr->type = htons(s - stats); osr->flags = 0; resize_openflow_skb(skb, &osr->header, max_openflow_len); @@@ -1465,8 -1437,6 +1438,6 @@@ err = skb->len; } - out: - rcu_read_unlock(); return err; } diff --combined datapath/hwtable_dummy/hwtable_dummy.c index b6c5832b,f6c45cdc..a80ce6b7 --- a/datapath/hwtable_dummy/hwtable_dummy.c +++ b/datapath/hwtable_dummy/hwtable_dummy.c @@@ -35,7 -35,6 +35,7 @@@ #include #include #include +#include #include #include @@@ -49,10 -48,6 +49,6 @@@ #define DUMMY_MAX_FLOW 8192 - /* xxx Explain need for this separate list because of RCU */ - static spinlock_t pending_free_lock; - static struct list_head pending_free_list; - /* sw_flow private data for dummy table entries. */ struct sw_flow_dummy { struct list_head node; @@@ -63,43 -58,14 +59,14 @@@ struct sw_table_dummy { struct sw_table swt; - spinlock_t lock; unsigned int max_flows; - atomic_t n_flows; + unsigned int n_flows; struct list_head flows; struct list_head iter_flows; unsigned long int next_serial; }; - static void table_dummy_sfw_destroy(struct sw_flow_dummy *sfw) - { - /* xxx Remove the entry from hardware. If you need to do any other - * xxx clean-up associated with the entry, do it here. - */ - - kfree(sfw); - } - - static void table_dummy_rcu_callback(struct rcu_head *rcu) - { - struct sw_flow *flow = container_of(rcu, struct sw_flow, rcu); - - spin_lock(&pending_free_lock); - if (flow->private) { - struct sw_flow_dummy *sfw = flow->private; - list_add(&sfw->node, &pending_free_list); - flow->private = NULL; - } - spin_unlock(&pending_free_lock); - flow_free(flow); - } - - static void table_dummy_flow_deferred_free(struct sw_flow *flow) - { - call_rcu(&flow->rcu, table_dummy_rcu_callback); - } - static struct sw_flow *table_dummy_lookup(struct sw_table *swt, const struct sw_flow_key *key) { @@@ -123,7 -89,8 +90,8 @@@ static int table_dummy_insert(struct sw /* xxx Do whatever needs to be done to insert an entry in hardware. * xxx If the entry can't be inserted, return 0. This stub code * xxx doesn't do anything yet, so we're going to return 0...you - * xxx shouldn't. + * xxx shouldn't (and you should update n_flows in struct + * xxx sw_table_dummy, too). */ kfree(flow->private); return 0; @@@ -132,13 -99,12 +100,12 @@@ static int do_delete(struct sw_table *swt, struct sw_flow *flow) { - if (flow_del(flow)) { - list_del_rcu(&flow->node); - list_del_rcu(&flow->iter_node); - table_dummy_flow_deferred_free(flow); - return 1; - } - return 0; + /* xxx Remove the entry from hardware. If you need to do any other + * xxx clean-up associated with the entry, do it here. + */ + list_del_rcu(&flow->node); + list_del_rcu(&flow->iter_node); + return 1; } static int table_dummy_delete(struct sw_table *swt, @@@ -148,13 -114,12 +115,12 @@@ struct sw_flow *flow; unsigned int count = 0; - list_for_each_entry_rcu (flow, &td->flows, node) { + list_for_each_entry (flow, &td->flows, node) { if (flow_del_matches(&flow->key, key, strict) && (!strict || (flow->priority == priority))) count += do_delete(swt, flow); } - if (count) - atomic_sub(count, &td->n_flows); + td->n_flows -= count; return count; } @@@ -163,12 -128,12 +129,12 @@@ static int table_dummy_timeout(struct d { struct sw_table_dummy *td = (struct sw_table_dummy *) swt; struct sw_flow *flow; - struct sw_flow_dummy *sfw, *n; int del_count = 0; uint64_t packet_count = 0; int i = 0; - list_for_each_entry_rcu (flow, &td->flows, node) { + mutex_lock(&dp_mutex); + list_for_each_entry (flow, &td->flows, node) { /* xxx Retrieve the packet count associated with this entry * xxx and store it in "packet_count". */ @@@ -187,22 -152,11 +153,11 @@@ } del_count += do_delete(swt, flow); } - if ((i % 50) == 0) { - msleep_interruptible(1); - } i++; } + mutex_unlock(&dp_mutex); - /* Remove any entries queued for removal */ - spin_lock_bh(&pending_free_lock); - list_for_each_entry_safe (sfw, n, &pending_free_list, node) { - list_del(&sfw->node); - table_dummy_sfw_destroy(sfw); - } - spin_unlock_bh(&pending_free_lock); - - if (del_count) - atomic_sub(del_count, &td->n_flows); + td->n_flows -= del_count; return del_count; } @@@ -239,7 -193,7 +194,7 @@@ static int table_dummy_iterate(struct s unsigned long start; start = ~position->private[0]; - list_for_each_entry_rcu (flow, &tl->iter_flows, iter_node) { + list_for_each_entry (flow, &tl->iter_flows, iter_node) { if (flow->serial <= start && flow_matches(key, &flow->key)) { int error = callback(flow, private); if (error) { @@@ -256,7 -210,7 +211,7 @@@ static void table_dummy_stats(struct sw { struct sw_table_dummy *td = (struct sw_table_dummy *) swt; stats->name = "dummy"; - stats->n_flows = atomic_read(&td->n_flows); + stats->n_flows = td->n_flows; stats->max_flows = td->max_flows; } @@@ -280,15 -234,11 +235,11 @@@ static struct sw_table *table_dummy_cre swt->stats = table_dummy_stats; td->max_flows = DUMMY_MAX_FLOW; - atomic_set(&td->n_flows, 0); + td->n_flows = 0; INIT_LIST_HEAD(&td->flows); INIT_LIST_HEAD(&td->iter_flows); - spin_lock_init(&td->lock); td->next_serial = 0; - INIT_LIST_HEAD(&pending_free_list); - spin_lock_init(&pending_free_lock); - return swt; } diff --combined datapath/table-linear.c index 80948387,a22776a3..b4d4a775 --- a/datapath/table-linear.c +++ b/datapath/table-linear.c @@@ -10,14 -10,13 +10,13 @@@ #include #include -#include +#include struct sw_table_linear { struct sw_table swt; - spinlock_t lock; unsigned int max_flows; - atomic_t n_flows; + unsigned int n_flows; struct list_head flows; struct list_head iter_flows; unsigned long int next_serial; @@@ -38,7 -37,6 +37,6 @@@ static struct sw_flow *table_linear_loo static int table_linear_insert(struct sw_table *swt, struct sw_flow *flow) { struct sw_table_linear *tl = (struct sw_table_linear *) swt; - unsigned long int flags; struct sw_flow *f; @@@ -46,16 -44,13 +44,13 @@@ * always be placed behind those with equal priority. Just replace * any flows that match exactly. */ - spin_lock_irqsave(&tl->lock, flags); - list_for_each_entry_rcu (f, &tl->flows, node) { + list_for_each_entry (f, &tl->flows, node) { if (f->priority == flow->priority && f->key.wildcards == flow->key.wildcards - && flow_matches(&f->key, &flow->key) - && flow_del(f)) { + && flow_matches(&f->key, &flow->key)) { flow->serial = f->serial; list_replace_rcu(&f->node, &flow->node); list_replace_rcu(&f->iter_node, &flow->iter_node); - spin_unlock_irqrestore(&tl->lock, flags); flow_deferred_free(f); return 1; } @@@ -65,29 -60,24 +60,24 @@@ } /* Make sure there's room in the table. */ - if (atomic_read(&tl->n_flows) >= tl->max_flows) { - spin_unlock_irqrestore(&tl->lock, flags); + if (tl->n_flows >= tl->max_flows) { return 0; } - atomic_inc(&tl->n_flows); + tl->n_flows++; /* Insert the entry immediately in front of where we're pointing. */ flow->serial = tl->next_serial++; list_add_tail_rcu(&flow->node, &f->node); list_add_rcu(&flow->iter_node, &tl->iter_flows); - spin_unlock_irqrestore(&tl->lock, flags); return 1; } static int do_delete(struct sw_table *swt, struct sw_flow *flow) { - if (flow_del(flow)) { - list_del_rcu(&flow->node); - list_del_rcu(&flow->iter_node); - flow_deferred_free(flow); - return 1; - } - return 0; + list_del_rcu(&flow->node); + list_del_rcu(&flow->iter_node); + flow_deferred_free(flow); + return 1; } static int table_linear_delete(struct sw_table *swt, @@@ -97,13 -87,12 +87,12 @@@ struct sw_flow *flow; unsigned int count = 0; - list_for_each_entry_rcu (flow, &tl->flows, node) { + list_for_each_entry (flow, &tl->flows, node) { if (flow_del_matches(&flow->key, key, strict) && (!strict || (flow->priority == priority))) count += do_delete(swt, flow); } - if (count) - atomic_sub(count, &tl->n_flows); + tl->n_flows -= count; return count; } @@@ -113,15 -102,16 +102,16 @@@ static int table_linear_timeout(struct struct sw_flow *flow; int count = 0; - list_for_each_entry_rcu (flow, &tl->flows, node) { + mutex_lock(&dp_mutex); + list_for_each_entry (flow, &tl->flows, node) { if (flow_timeout(flow)) { count += do_delete(swt, flow); if (dp->flags & OFPC_SEND_FLOW_EXP) dp_send_flow_expired(dp, flow); } } - if (count) - atomic_sub(count, &tl->n_flows); + tl->n_flows -= count; + mutex_unlock(&dp_mutex); return count; } @@@ -149,7 -139,7 +139,7 @@@ static int table_linear_iterate(struct unsigned long start; start = position->private[0]; - list_for_each_entry_rcu (flow, &tl->iter_flows, iter_node) { + list_for_each_entry (flow, &tl->iter_flows, iter_node) { if (flow->serial >= start && flow_matches(key, &flow->key)) { int error = callback(flow, private); if (error) { @@@ -166,7 -156,7 +156,7 @@@ static void table_linear_stats(struct s { struct sw_table_linear *tl = (struct sw_table_linear *) swt; stats->name = "linear"; - stats->n_flows = atomic_read(&tl->n_flows); + stats->n_flows = tl->n_flows; stats->max_flows = tl->max_flows; } @@@ -190,10 -180,9 +180,9 @@@ struct sw_table *table_linear_create(un swt->stats = table_linear_stats; tl->max_flows = max_flows; - atomic_set(&tl->n_flows, 0); + tl->n_flows = 0; INIT_LIST_HEAD(&tl->flows); INIT_LIST_HEAD(&tl->iter_flows); - spin_lock_init(&tl->lock); tl->next_serial = 0; return swt;