On Mon, Mar 31, 2014 at 11:18 AM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
> ---
> v6: Use key fields of the newly allocated flow directly in ovs_flow_cmd_new().
> Correctly handle the case of no acts in ovs_flow_cmd_set().
>
> datapath/datapath.c | 193
> ++++++++++++++++++++++++++++++---------------------
> 1 file changed, 115 insertions(+), 78 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index f88147e..52c76b4 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -798,8 +798,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;
> @@ -807,61 +806,76 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct
> genl_info *info)
> struct sw_flow_match match;
> int error;
>
> - /* Extract key. */
> + /* Must have key and actions. */
> error = -EINVAL;
> if (!a[OVS_FLOW_ATTR_KEY])
> goto error;
> + if (!a[OVS_FLOW_ATTR_ACTIONS])
> + goto error;
>
> - ovs_match_init(&match, &key, &mask);
> + /* 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 error;
> + }
> +
> + /* Extract key. */
> + ovs_match_init(&match, &new_flow->unmasked_key, &mask);
> error = ovs_nla_get_match(&match,
> a[OVS_FLOW_ATTR_KEY],
> a[OVS_FLOW_ATTR_MASK]);
> if (error)
> - goto error;
> + goto err_kfree_flow;
>
> - /* Validate actions. */
> - error = -EINVAL;
> - if (!a[OVS_FLOW_ATTR_ACTIONS])
> - goto error;
> + ovs_flow_mask_key(&new_flow->key, &new_flow->unmasked_key, &mask);
>
> + /* Validate actions. */
> acts = ovs_nla_alloc_flow_actions(nla_len(a[OVS_FLOW_ATTR_ACTIONS]));
> error = PTR_ERR(acts);
> if (IS_ERR(acts))
> - goto error;
> + goto err_kfree_flow;
>
> - ovs_flow_mask_key(&masked_key, &key, &mask);
> - error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS],
> - &masked_key, 0, &acts);
> + error = ovs_nla_copy_actions(a[OVS_FLOW_ATTR_ACTIONS], &new_flow->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)) {
> + error = -ENODEV;
> 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);
> + flow = ovs_flow_tbl_lookup(&dp->table, &new_flow->unmasked_key);
> + 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;
>
> @@ -871,40 +885,45 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct
> genl_info *info)
> * request. We also accept NLM_F_EXCL in case that bug ever
> * 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))) {
> + error = -EEXIST;
> 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))) {
> + error = -EEXIST;
> 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);
> }
How much do we save by unlocking ovs-lock before free, I think this is
rare case. If so then we can move ovs-unlock out of if and else block.
It is just easy to read.
> +
> + 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:
> + kfree_skb(reply);
> +err_kfree_acts:
> kfree(acts);
> +err_kfree_flow:
> + ovs_flow_free(new_flow, false);
> error:
> return error;
> }
> @@ -918,7 +937,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;
>
> @@ -945,56 +964,74 @@ 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;
> + }
> + }
> +
> + /* Can allocate before locking if have acts. */
> + if (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)) {
> + error = -ENODEV;
> goto err_unlock_ovs;
> -
> + }
> /* Check that the flow exists. */
> flow = ovs_flow_tbl_lookup(&dp->table, &key);
> - error = -ENOENT;
> - if (!flow)
> + if (unlikely(!flow)) {
> + error = -ENOENT;
> goto err_unlock_ovs;
> -
> + }
> /* The unmasked key has to be the same for flow updates. */
> - error = -EEXIST;
> - if (!ovs_flow_cmp_unmasked_key(flow, &match))
> + if (unlikely(!ovs_flow_cmp_unmasked_key(flow, &match))) {
> + error = -EEXIST;
> goto err_unlock_ovs;
> -
> + }
> /* Update actions, if present. */
> - if (acts) {
> - struct sw_flow_actions *old_acts;
> -
> + if (likely(acts)) {
> old_acts = ovsl_dereference(flow->sf_acts);
> rcu_assign_pointer(flow->sf_acts, acts);
> - ovs_nla_free_flow_actions(old_acts);
> +
> + 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);
> + }
> + } else {
> + /* Could not alloc without acts before locking. */
> + reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
> + info, OVS_FLOW_CMD_NEW,
> false);
> + if (unlikely(IS_ERR(reply))) {
> + error = PTR_ERR(reply);
> + goto err_unlock_ovs;
> + }
> }
>
> - reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
> - info, OVS_FLOW_CMD_NEW, false);
> /* 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;
Thanks for simplifying and improving datapath locking
Acked-by: Pravin B Shelar <pshe...@nicira.com>
> --
> 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
