On Mon, Mar 24, 2014 at 11:56 AM, Jarno Rajahalme <jrajaha...@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 <jrajaha...@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;
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
