[ovs-dev] [PATCH v4 4/7] datapath: Minimize ovs_flow_cmd_del critical section.
Pravin Shelar
pshelar at nicira.com
Mon Mar 24 21:42:05 UTC 2014
On Mon, Mar 24, 2014 at 11:56 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> ovs_flow_cmd_del() now allocates reply (if needed) after the flow has
> already been removed from the flow table. If the reply allocation
> fails, a netlink error is signaled with netlink_set_err(), as is
> already done in ovs_flow_cmd_new_or_set() in the similar situation.
>
This patch does two different optimizations which are not related. Can
you separate them?
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> ---
> datapath/datapath.c | 70 +++++++++++++++++++++++++--------------------------
> 1 file changed, 34 insertions(+), 36 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index f933831..e7cfd40 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -664,7 +664,7 @@ static size_t ovs_flow_cmd_msg_size(const struct sw_flow_actions *acts)
> }
>
> /* Called with ovs_mutex or RCU read lock. */
> -static int ovs_flow_cmd_fill_info(struct sw_flow *flow, struct datapath *dp,
> +static int ovs_flow_cmd_fill_info(struct sw_flow *flow, int dp_ifindex,
> struct sk_buff *skb, u32 portid,
> u32 seq, u32 flags, u8 cmd)
> {
> @@ -681,7 +681,7 @@ static int ovs_flow_cmd_fill_info(struct sw_flow *flow, struct datapath *dp,
> if (!ovs_header)
> return -EMSGSIZE;
>
> - ovs_header->dp_ifindex = get_dpifindex(dp);
> + ovs_header->dp_ifindex = dp_ifindex;
>
> /* Fill flow key. */
> nla = nla_nest_start(skb, OVS_FLOW_ATTR_KEY);
> @@ -767,9 +767,8 @@ static struct sk_buff *ovs_flow_cmd_alloc_info(struct sw_flow *flow,
>
> /* Must be called with ovs_mutex. */
> static struct sk_buff *ovs_flow_cmd_build_info(struct sw_flow *flow,
> - struct datapath *dp,
> - struct genl_info *info,
> - u8 cmd)
> + int dp_ifindex,
> + struct genl_info *info, u8 cmd)
> {
> struct sk_buff *skb;
> int retval;
> @@ -778,8 +777,9 @@ static struct sk_buff *ovs_flow_cmd_build_info(struct sw_flow *flow,
> if (!skb)
> return ERR_PTR(-ENOMEM);
>
> - retval = ovs_flow_cmd_fill_info(flow, dp, skb, info->snd_portid,
> - info->snd_seq, 0, cmd);
> + retval = ovs_flow_cmd_fill_info(flow, dp_ifindex, skb,
> + info->snd_portid, info->snd_seq, 0,
> + cmd);
> BUG_ON(retval < 0);
> return skb;
> }
> @@ -860,7 +860,9 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
> goto err_flow_free;
>
> if (ovs_must_build_reply(info, &ovs_dp_flow_multicast_group))
> - reply = ovs_flow_cmd_build_info(flow, dp, info,
> + reply = ovs_flow_cmd_build_info(flow,
> + ovs_header->dp_ifindex,
> + info,
> OVS_FLOW_CMD_NEW);
> } else {
> /* We found a matching flow. */
> @@ -889,7 +891,9 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
> }
>
> if (ovs_must_build_reply(info, &ovs_dp_flow_multicast_group))
> - reply = ovs_flow_cmd_build_info(flow, dp, info,
> + reply = ovs_flow_cmd_build_info(flow,
> + ovs_header->dp_ifindex,
> + info,
> OVS_FLOW_CMD_NEW);
>
> /* Clear stats. */
> @@ -952,7 +956,8 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct genl_info *info)
> goto unlock;
> }
>
> - reply = ovs_flow_cmd_build_info(flow, dp, info, OVS_FLOW_CMD_NEW);
> + reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex, info,
> + OVS_FLOW_CMD_NEW);
> if (IS_ERR(reply)) {
> err = PTR_ERR(reply);
> goto unlock;
> @@ -970,57 +975,50 @@ static int ovs_flow_cmd_del(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;
> - struct sk_buff *reply = NULL;
> struct sw_flow *flow;
> struct datapath *dp;
> struct sw_flow_match match;
> int err;
>
> + if (a[OVS_FLOW_ATTR_KEY]) {
> + ovs_match_init(&match, &key, NULL);
> + err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL);
> + if (err)
> + return err;
> + }
> +
> ovs_lock();
> dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
> if (!dp) {
> err = -ENODEV;
> goto unlock;
> }
> -
> if (!a[OVS_FLOW_ATTR_KEY]) {
> err = ovs_flow_tbl_flush(&dp->table);
> goto unlock;
> }
> -
> - ovs_match_init(&match, &key, NULL);
> - err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL);
> - if (err)
> - goto unlock;
> -
> flow = ovs_flow_tbl_lookup(&dp->table, &key);
> if (!flow || !ovs_flow_cmp_unmasked_key(flow, &match)) {
> err = -ENOENT;
> goto unlock;
> }
> + ovs_flow_tbl_remove(&dp->table, flow);
> + ovs_unlock();
>
> if (ovs_must_build_reply(info, &ovs_dp_flow_multicast_group)) {
> - reply = ovs_flow_cmd_alloc_info(flow, info);
> - if (!reply) {
> - err = -ENOMEM;
> - goto unlock;
> - }
> - }
> -
> - ovs_flow_tbl_remove(&dp->table, flow);
> + struct sk_buff *reply;
>
> - if (reply) {
> - err = ovs_flow_cmd_fill_info(flow, dp, reply, info->snd_portid,
> - info->snd_seq, 0,
> - OVS_FLOW_CMD_DEL);
> - BUG_ON(err < 0);
> + reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
> + info, OVS_FLOW_CMD_DEL);
> + 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_flow_free(flow, true);
> - ovs_unlock();
This patch is hitting assert in ovs_flow_free().
Plus I am seeing RCU checks failing in function building reply.
Simplest fix would be not moving unlock function call.
> -
> - if (reply)
> - ovs_notify(reply, info, &ovs_dp_flow_multicast_group);
> return 0;
> unlock:
> ovs_unlock();
> @@ -1051,7 +1049,7 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
> if (!flow)
> break;
>
> - if (ovs_flow_cmd_fill_info(flow, dp, skb,
> + if (ovs_flow_cmd_fill_info(flow, ovs_header->dp_ifindex, skb,
> NETLINK_CB(cb->skb).portid,
> cb->nlh->nlmsg_seq, NLM_F_MULTI,
> OVS_FLOW_CMD_NEW) < 0)
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list