On Tue, Mar 25, 2014 at 2:35 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
> ---
> v5: Make functionality after ovs_unlock() case specific to make the common
> case more optimal.
>
> datapath/datapath.c | 135
> ++++++++++++++++++++++++++++++---------------------
> 1 file changed, 81 insertions(+), 54 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 0de00ad..aa3f789 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -808,7 +808,7 @@ 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;
> struct datapath *dp;
> @@ -842,35 +842,52 @@ 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;
> + }
> +
> + reply = ovs_flow_cmd_alloc_info(acts, info, false);
> + if (IS_ERR(reply)) {
> + error = PTR_ERR(reply);
> + 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;
> +
If flow alloc is done at start, we can directly use flow->key and
flow->unmasked_key rather than using local key and unmasked_key.
> ovs_lock();
> dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
> error = -ENODEV;
> - if (!dp)
> + if (unlikely(!dp))
> goto err_unlock_ovs;
>
> /* 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;
> - rcu_assign_pointer(flow->sf_acts, acts);
> + if (likely(!flow)) {
> + rcu_assign_pointer(new_flow->sf_acts, acts);
>
> /* Put flow in bucket. */
> - error = ovs_flow_tbl_insert(&dp->table, flow, &mask);
> - if (error) {
> + error = ovs_flow_tbl_insert(&dp->table, new_flow, &mask);
> + if (unlikely(error)) {
> acts = NULL;
> - goto err_flow_free;
> + goto err_unlock_ovs;
> }
> +
> + if (unlikely(reply)) {
> + error = ovs_flow_cmd_fill_info(new_flow,
> + ovs_header->dp_ifindex,
> + reply,
> info->snd_portid,
> + info->snd_seq, 0,
> + OVS_FLOW_CMD_NEW);
> + BUG_ON(error < 0);
> + }
> + ovs_unlock();
> } else {
> struct sw_flow_actions *old_acts;
>
> @@ -881,38 +898,42 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct
> genl_info *info)
> * gets fixed.
> */
> error = -EEXIST;
> - if (info->nlhdr->nlmsg_flags & (NLM_F_CREATE | NLM_F_EXCL))
> + if (unlikely(info->nlhdr->nlmsg_flags & (NLM_F_CREATE
> + | NLM_F_EXCL)))
> goto err_unlock_ovs;
>
> /* The unmasked key has to be the same for flow updates. */
> - if (!ovs_flow_cmp_unmasked_key(flow, &match))
> + if (unlikely(!ovs_flow_cmp_unmasked_key(flow, &match)))
> goto err_unlock_ovs;
>
> /* 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, false);
> - ovs_unlock();
> + if (unlikely(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) {
> - 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_nla_free_flow_actions(old_acts);
> + ovs_flow_free(new_flow, false);
> }
> +
> + if (reply)
> + ovs_notify(reply, info, &ovs_dp_flow_multicast_group);
> return 0;
>
> -err_flow_free:
> - ovs_flow_free(flow, false);
> err_unlock_ovs:
> ovs_unlock();
> -err_kfree:
> + ovs_flow_free(new_flow, false);
> +err_kfree_reply:
> + kfree_skb(reply);
> +err_kfree_acts:
> kfree(acts);
> error:
> return error;
> @@ -927,7 +948,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;
>
> @@ -954,55 +975,61 @@ 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;
> }
> }
>
> + reply = ovs_flow_cmd_alloc_info(acts, info, false);
> + if (IS_ERR(reply)) {
> + error = PTR_ERR(reply);
> + goto err_kfree_acts;
> + }
> +
> ovs_lock();
> dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
> error = -ENODEV;
> - if (!dp)
> + if (unlikely(!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)
> + if (unlikely(!flow))
> goto err_unlock_ovs;
>
> /* The unmasked key has to be the same for flow updates. */
> - if (!ovs_flow_cmp_unmasked_key(flow, &match))
> + if (unlikely(!ovs_flow_cmp_unmasked_key(flow, &match)))
> goto err_unlock_ovs;
>
> /* 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);
> }
>
> - reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
> - info, OVS_FLOW_CMD_NEW, false);
> + 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
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
