[ovs-dev] [PATCH v4 6/7] datapath: Minimize ovs_flow_cmd_new_or_set critical section.
Jarno Rajahalme
jrajahalme at nicira.com
Mon Mar 24 18:56:05 UTC 2014
Note that this has only a little performance benefit when responses
are not created (which is normal when there are no netlink multicast
listeners around).
Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
---
datapath/datapath.c | 127 ++++++++++++++++++++++++++++-----------------------
1 file changed, 71 insertions(+), 56 deletions(-)
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 809a9c4..6a3d155 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -754,15 +754,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. */
@@ -773,7 +769,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);
@@ -789,11 +785,11 @@ static int ovs_flow_cmd_new(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;
+ struct sw_flow *flow, *new_flow;
struct sw_flow_mask mask;
struct sk_buff *reply = NULL;
struct datapath *dp;
- struct sw_flow_actions *acts;
+ struct sw_flow_actions *acts, *old_acts = NULL;
struct sw_flow_match match;
int error;
@@ -823,9 +819,27 @@ static int ovs_flow_cmd_new(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;
+ }
+
+ if (ovs_must_build_reply(info, &ovs_dp_flow_multicast_group)) {
+ reply = ovs_flow_cmd_alloc_info(acts, info);
+ if (!reply) {
+ error = -ENOMEM;
+ goto err_kfree_acts;
+ }
}
+ /* Most of the time we need to allocate a new flow, do it before
+ * locking. */
+ new_flow = ovs_flow_alloc();
+ if (IS_ERR(new_flow)) {
+ error = PTR_ERR(new_flow);
+ goto err_kfree_reply;
+ }
+ new_flow->key = masked_key;
+ new_flow->unmasked_key = key;
+
ovs_lock();
dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
error = -ENODEV;
@@ -835,25 +849,16 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
/* Check if this is a duplicate flow */
flow = ovs_flow_tbl_lookup(&dp->table, &key);
if (!flow) {
- /* Allocate flow. */
- flow = ovs_flow_alloc();
- if (IS_ERR(flow)) {
- error = PTR_ERR(flow);
- goto err_unlock_ovs;
- }
-
- flow->key = masked_key;
- flow->unmasked_key = key;
+ flow = new_flow;
rcu_assign_pointer(flow->sf_acts, acts);
acts = NULL;
/* Put flow in bucket. */
error = ovs_flow_tbl_insert(&dp->table, flow, &mask);
if (error)
- goto err_flow_free;
+ goto err_unlock_ovs;
+ new_flow = NULL;
} else {
- 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
@@ -871,29 +876,32 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
/* Update actions. */
old_acts = ovsl_dereference(flow->sf_acts);
rcu_assign_pointer(flow->sf_acts, acts);
- ovs_nla_free_flow_actions(old_acts);
}
- if (ovs_must_build_reply(info, &ovs_dp_flow_multicast_group))
- reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
- info, OVS_FLOW_CMD_NEW);
- ovs_unlock();
-
if (reply) {
- 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));
+ 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 (reply)
+ ovs_notify(reply, info, &ovs_dp_flow_multicast_group);
+ if (new_flow)
+ ovs_flow_free(new_flow, false);
+ if (old_acts)
+ ovs_nla_free_flow_actions(old_acts);
return 0;
-err_flow_free:
- ovs_flow_free(flow, false);
err_unlock_ovs:
ovs_unlock();
-err_kfree:
+ if (new_flow)
+ ovs_flow_free(new_flow, false);
+err_kfree_reply:
+ kfree_skb(reply);
+err_kfree_acts:
kfree(acts);
error:
return error;
@@ -908,7 +916,7 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
struct sw_flow_mask mask;
struct sk_buff *reply = NULL;
struct datapath *dp;
- struct sw_flow_actions *acts = NULL;
+ struct sw_flow_actions *old_acts = NULL, *acts = NULL;
struct sw_flow_match match;
int error;
@@ -935,7 +943,15 @@ static int ovs_flow_cmd_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;
+ }
+ }
+
+ if (ovs_must_build_reply(info, &ovs_dp_flow_multicast_group)) {
+ reply = ovs_flow_cmd_alloc_info(acts, info);
+ if (!reply) {
+ error = -ENOMEM;
+ goto err_kfree_acts;
}
}
@@ -945,7 +961,7 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
if (!dp)
goto err_unlock_ovs;
- /* Check if this is a duplicate flow */
+ /* Check if the flow exists. */
flow = ovs_flow_tbl_lookup(&dp->table, &key);
error = -ENOENT;
if (!flow)
@@ -957,34 +973,33 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
/* Update actions, if present. */
if (acts) {
- struct sw_flow_actions *old_acts;
-
old_acts = ovsl_dereference(flow->sf_acts);
rcu_assign_pointer(flow->sf_acts, acts);
- ovs_nla_free_flow_actions(old_acts);
}
- if (ovs_must_build_reply(info, &ovs_dp_flow_multicast_group))
- reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
- info, OVS_FLOW_CMD_NEW);
+ if (reply) {
+ 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);
+ }
+
/* Clear stats. */
if (a[OVS_FLOW_ATTR_CLEAR])
ovs_flow_stats_clear(flow);
ovs_unlock();
- if (reply) {
- 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));
- }
+ if (reply)
+ ovs_notify(reply, info, &ovs_dp_flow_multicast_group);
+ if (old_acts)
+ ovs_nla_free_flow_actions(old_acts);
return 0;
err_unlock_ovs:
ovs_unlock();
-err_kfree:
+ kfree_skb(reply);
+err_kfree_acts:
kfree(acts);
error:
return error;
--
1.7.10.4
More information about the dev
mailing list