[ovs-dev] [PATCH v4 3/7] datapath: Build flow cmd netlink reply only if needed.

[Pravin Shelar]

On Mon, Mar 24, 2014 at 11:56 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> Use netlink_has_listeners() and NLM_F_ECHO flag to determine if a
> reply is needed or not for OVS_FLOW_CMD_NEW, OVS_FLOW_CMD_SET, or
> OVS_FLOW_CMD_DEL.
>
> Note: This may need compat support for older kernels.
>
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> ---
>  datapath/datapath.c |   57 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 40 insertions(+), 17 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 7ce3ea6..f933831 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -64,6 +64,15 @@
>
>  int ovs_net_id __read_mostly;
>
> +/* Check if need to build a reply message.
> + * OVS userspace sets the NLM_F_ECHO flag if it needs the reply. */
> +static bool ovs_must_build_reply(struct genl_info *info,
> +                                const struct genl_multicast_group *grp)
> +{
> +       return info->nlhdr->nlmsg_flags & NLM_F_ECHO ||
> +               netlink_has_listeners(genl_info_net(info)->genl_sock, grp->id);
> +}
> +
>  static void ovs_notify(struct sk_buff *skb, struct genl_info *info,
>                        struct genl_multicast_group *grp)
>  {
> @@ -782,7 +791,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
>         struct sw_flow_key key, masked_key;
>         struct sw_flow *flow = NULL;
>         struct sw_flow_mask mask;
> -       struct sk_buff *reply;
> +       struct sk_buff *reply = NULL;
>         struct datapath *dp;
>         struct sw_flow_actions *acts = NULL;
>         struct sw_flow_match match;
> @@ -850,7 +859,9 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
>                 if (error)
>                         goto err_flow_free;
>
> -               reply = ovs_flow_cmd_build_info(flow, dp, info, OVS_FLOW_CMD_NEW);
> +               if (ovs_must_build_reply(info, &ovs_dp_flow_multicast_group))
> +                       reply = ovs_flow_cmd_build_info(flow, dp, info,
> +                                                       OVS_FLOW_CMD_NEW);
Can you move this check to ovs_flow_cmd_alloc_info()? this makes code
easy to read.

>         } else {
>                 /* We found a matching flow. */
>                 /* Bail out if we're not allowed to modify an existing flow.
> @@ -876,7 +887,10 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
>                         rcu_assign_pointer(flow->sf_acts, acts);
>                         ovs_nla_free_flow_actions(old_acts);
>                 }
> -               reply = ovs_flow_cmd_build_info(flow, dp, info, OVS_FLOW_CMD_NEW);
> +
> +               if (ovs_must_build_reply(info, &ovs_dp_flow_multicast_group))
> +                       reply = ovs_flow_cmd_build_info(flow, dp, info,
> +                                                       OVS_FLOW_CMD_NEW);
>
>                 /* Clear stats. */
>                 if (a[OVS_FLOW_ATTR_CLEAR])
> @@ -884,11 +898,14 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, struct genl_info *info)
>         }
>         ovs_unlock();
>
> -       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) {
> +               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));
> +       }
>         return 0;
>
>  err_flow_free:
> @@ -953,7 +970,7 @@ 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;
> +       struct sk_buff *reply = NULL;
>         struct sw_flow *flow;
>         struct datapath *dp;
>         struct sw_flow_match match;
> @@ -982,22 +999,28 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct genl_info *info)
>                 goto unlock;
>         }
>
> -       reply = ovs_flow_cmd_alloc_info(flow, info);
> -       if (!reply) {
> -               err = -ENOMEM;
> -               goto 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);
>
> -       err = ovs_flow_cmd_fill_info(flow, dp, reply, info->snd_portid,
> -                                    info->snd_seq, 0, OVS_FLOW_CMD_DEL);
> -       BUG_ON(err < 0);
> +       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);
> +       }
>
>         ovs_flow_free(flow, true);
>         ovs_unlock();
>
> -       ovs_notify(reply, info, &ovs_dp_flow_multicast_group);
> +       if (reply)
> +               ovs_notify(reply, info, &ovs_dp_flow_multicast_group);
>         return 0;
>  unlock:
>         ovs_unlock();
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev