[ovs-dev] [PATCH v2 09/13] datapath: Minimize ovs_flow_cmd_new_or_set critical section.

Jarno Rajahalme jrajahalme at nicira.com
Wed Feb 12 00:07:20 UTC 2014


Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
---
 datapath/datapath.c   |   83 ++++++++++++++++++++++++-------------------------
 datapath/flow_table.c |    2 +-
 datapath/flow_table.h |    2 +-
 3 files changed, 43 insertions(+), 44 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index cbdd68f..1599d70 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -745,15 +745,11 @@ error:
 	return err;
 }
 
-/* Must be called with ovs_mutex. */
-static struct sk_buff *ovs_flow_cmd_alloc_info(struct sw_flow *flow,
+static struct sk_buff *ovs_flow_cmd_alloc_info(const struct sw_flow_actions *acts,
 					       struct genl_info *info)
 {
-	size_t len;
-
-	len = ovs_flow_cmd_msg_size(ovsl_dereference(flow->sf_acts));
-
-	return genlmsg_new_unicast(len, info, GFP_KERNEL);
+	return genlmsg_new_unicast(ovs_flow_cmd_msg_size(acts), info,
+				   GFP_KERNEL);
 }
 
 /* Must be called with ovs_mutex. */
@@ -764,7 +760,7 @@ static struct sk_buff *ovs_flow_cmd_build_info(struct sw_flow *flow,
 	struct sk_buff *skb;
 	int retval;
 
-	skb = ovs_flow_cmd_alloc_info(flow, info);
+	skb = ovs_flow_cmd_alloc_info(ovsl_dereference(flow->sf_acts), info);
 	if (!skb)
 		return ERR_PTR(-ENOMEM);
 
@@ -780,13 +776,14 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
 	struct nlattr **a = info->attrs;
 	struct ovs_header *ovs_header = info->userhdr;
 	struct sw_flow_key key, masked_key;
-	struct sw_flow *flow = NULL;
+	struct sw_flow *flow;
 	struct sw_flow_mask mask;
 	struct sk_buff *reply;
 	struct datapath *dp;
 	struct sw_flow_actions *acts = NULL;
 	struct sw_flow_match match;
 	int error;
+	u8 cmd = info->genlhdr->cmd;
 
 	/* Extract key. */
 	error = -EINVAL;
@@ -811,7 +808,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
 					     &masked_key, 0, &acts);
 		if (error) {
 			OVS_NLERR("Flow actions may not be safe on all matching packets.\n");
-			goto err_kfree;
+			goto err_kfree_acts;
 		}
 	} else if (info->genlhdr->cmd == OVS_FLOW_CMD_SET) {
 		/* Need empty actions. */
@@ -825,84 +822,86 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
 		goto error;
 	}
 
+	reply = ovs_flow_cmd_alloc_info(acts, info);
+	if (!reply) {
+		error = -ENOMEM;
+		goto err_kfree_acts;
+	}
+
 	ovs_lock();
 	dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
 	error = -ENODEV;
 	if (!dp)
-		goto err_unlock_ovs;
+		goto err_unlock;
 
 	/* Check if this is a duplicate flow */
 	flow = ovs_flow_tbl_lookup(&dp->table, &key);
 	if (!flow) {
 		/* Bail out if we're not allowed to create a new flow. */
 		error = -ENOENT;
-		if (info->genlhdr->cmd == OVS_FLOW_CMD_SET)
-			goto err_unlock_ovs;
+		if (cmd == OVS_FLOW_CMD_SET)
+			goto err_unlock;
 
 		/* Allocate flow. */
 		flow = ovs_flow_alloc();
 		if (IS_ERR(flow)) {
 			error = PTR_ERR(flow);
-			goto err_unlock_ovs;
+			goto err_unlock;
 		}
-
 		flow->key = masked_key;
 		flow->unmasked_key = key;
 		rcu_assign_pointer(flow->sf_acts, acts);
+		acts = NULL;
 
 		/* Put flow in bucket. */
 		error = ovs_flow_tbl_insert(&dp->table, flow, &mask);
+
 		if (error) {
-			acts = NULL;
-			goto err_flow_free;
+			ovs_flow_free(flow, false);
+			goto err_unlock;
 		}
-
-		reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
-						info, OVS_FLOW_CMD_NEW);
 	} else {
 		/* We found a matching flow. */
 		struct sw_flow_actions *old_acts;
 
-		/* Bail out if we're not allowed to modify an existing flow.
-		 * We accept NLM_F_CREATE in place of the intended NLM_F_EXCL
-		 * because Generic Netlink treats the latter as a dump
-		 * request.  We also accept NLM_F_EXCL in case that bug ever
-		 * gets fixed.
+		/* Bail out if we're not allowed to modify an
+		 * existing flow.  We accept NLM_F_CREATE in
+		 * place of the intended NLM_F_EXCL because
+		 * Generic Netlink treats the latter as a dump
+		 * request.  We also accept NLM_F_EXCL in case
+		 * that bug ever gets fixed.
 		 */
 		error = -EEXIST;
-		if (info->genlhdr->cmd == OVS_FLOW_CMD_NEW &&
-		    info->nlhdr->nlmsg_flags & (NLM_F_CREATE | NLM_F_EXCL))
-			goto err_unlock_ovs;
-
+		if (cmd == OVS_FLOW_CMD_NEW
+		    && info->nlhdr->nlmsg_flags & (NLM_F_CREATE | NLM_F_EXCL))
+			goto err_unlock;
 		/* The unmasked key has to be the same for flow updates. */
 		if (!ovs_flow_cmp_unmasked_key(flow, &match))
-			goto err_unlock_ovs;
+			goto err_unlock;
 
 		/* Update actions. */
 		old_acts = ovsl_dereference(flow->sf_acts);
 		rcu_assign_pointer(flow->sf_acts, acts);
 		ovs_nla_free_flow_actions(old_acts);
 
-		reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
-						info, OVS_FLOW_CMD_NEW);
 		/* Clear stats. */
 		if (a[OVS_FLOW_ATTR_CLEAR])
 			ovs_flow_stats_clear(flow);
 	}
+
+	error = ovs_flow_cmd_fill_info(flow, ovs_header->dp_ifindex, reply,
+				       info->snd_portid, info->snd_seq, 0,
+				       OVS_FLOW_CMD_NEW);
+	BUG_ON(error < 0);
 	ovs_unlock();
 
-	if (!IS_ERR(reply))
-		ovs_notify(reply, info, &ovs_dp_flow_multicast_group);
-	else
-		netlink_set_err(sock_net(skb->sk)->genl_sock, 0,
-				ovs_dp_flow_multicast_group.id,	PTR_ERR(reply));
+	ovs_notify(reply, info, &ovs_dp_flow_multicast_group);
 	return 0;
 
-err_flow_free:
-	ovs_flow_free(flow, false);
-err_unlock_ovs:
+err_unlock:
 	ovs_unlock();
-err_kfree:
+	kfree_skb(reply);
+err_kfree_acts:
 	kfree(acts);
 error:
 	return error;
@@ -993,7 +992,7 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
 	ovs_flow_tbl_remove(&dp->table, flow);
 	ovs_unlock();
 
-	reply = ovs_flow_cmd_alloc_info(flow, info);
+	reply = ovs_flow_cmd_alloc_info(ovsl_dereference(flow->sf_acts), info);
 	/* XXX: We may skip sending a response if kernel is out of memory.
 	 * We could do the allocation before locking if we did not need
 	 * to return the actions (which are useless in this case anyway). */
diff --git a/datapath/flow_table.c b/datapath/flow_table.c
index c04fb4d..b9c7ef0 100644
--- a/datapath/flow_table.c
+++ b/datapath/flow_table.c
@@ -422,7 +422,7 @@ static bool flow_cmp_masked_key(const struct sw_flow *flow,
 }
 
 bool ovs_flow_cmp_unmasked_key(const struct sw_flow *flow,
-			       struct sw_flow_match *match)
+			       const struct sw_flow_match *match)
 {
 	struct sw_flow_key *key = match->key;
 	int key_start = flow_key_start(key);
diff --git a/datapath/flow_table.h b/datapath/flow_table.h
index ca8a582..e3ab93d 100644
--- a/datapath/flow_table.h
+++ b/datapath/flow_table.h
@@ -78,7 +78,7 @@ struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *,
 				    const struct sw_flow_key *);
 
 bool ovs_flow_cmp_unmasked_key(const struct sw_flow *flow,
-			       struct sw_flow_match *match);
+			       const struct sw_flow_match *match);
 
 void ovs_flow_mask_key(struct sw_flow_key *dst, const struct sw_flow_key *src,
 		       const struct sw_flow_mask *mask);
-- 
1.7.10.4




More information about the dev mailing list