[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