[ovs-dev] [PATCH v4 6/7] datapath: Minimize ovs_flow_cmd_new_or_set critical section.

[Pravin Shelar]

On Mon, Mar 24, 2014 at 11:56 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> 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);
Most of time new_flow should be NULL, so we can move this check to error code.

> +       if (old_acts)
> +               ovs_nla_free_flow_actions(old_acts);
If we move old_acts to its respective case we can avoid checking it in
common case.
>         return 0;
>