On Tue, Mar 25, 2014 at 2:35 PM, Jarno Rajahalme <jrajaha...@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. Currently, OVS userspace does not request a reply
> for OVS_FLOW_CMD_NEW, but usually does for OVS_FLOW_CMD_DEL, as stats
> may have changed.
>
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
LGTM
Acked-by: Pravin B Shelar <pshe...@nicira.com>
> ---
> v5: Moved the check to ovs_flow_cmd_alloc_info(), added info about current
> OVS userspace to the commit message.
>
> datapath/datapath.c | 70
> +++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 49 insertions(+), 21 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 93f8e36..d1b0155 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_notify(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)
> {
> @@ -747,27 +756,37 @@ error:
>
> /* Must be called with ovs_mutex. */
> static struct sk_buff *ovs_flow_cmd_alloc_info(struct sw_flow *flow,
> - struct genl_info *info)
> + struct genl_info *info,
> + bool always)
> {
> + struct sk_buff *skb;
> size_t len;
>
> + if (!always && !ovs_must_notify(info, &ovs_dp_flow_multicast_group))
> + return NULL;
> +
> len = ovs_flow_cmd_msg_size(ovsl_dereference(flow->sf_acts));
>
> - return genlmsg_new_unicast(len, info, GFP_KERNEL);
> + skb = genlmsg_new_unicast(len, info, GFP_KERNEL);
> +
> + if (!skb)
> + return ERR_PTR(-ENOMEM);
> +
> + return skb;
> }
>
> /* 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)
> + u8 cmd, bool always)
> {
> struct sk_buff *skb;
> int retval;
>
> - skb = ovs_flow_cmd_alloc_info(flow, info);
> - if (!skb)
> - return ERR_PTR(-ENOMEM);
> + skb = ovs_flow_cmd_alloc_info(flow, info, always);
> + if (!skb || IS_ERR(skb))
> + return skb;
>
> retval = ovs_flow_cmd_fill_info(flow, dp, skb, info->snd_portid,
> info->snd_seq, 0, cmd);
> @@ -851,7 +870,8 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb,
> struct genl_info *info)
> goto err_flow_free;
> }
>
> - reply = ovs_flow_cmd_build_info(flow, dp, info,
> OVS_FLOW_CMD_NEW);
> + reply = ovs_flow_cmd_build_info(flow, dp, info,
> + OVS_FLOW_CMD_NEW, false);
> } else {
> /* We found a matching flow. */
> /* Bail out if we're not allowed to modify an existing flow.
> @@ -877,7 +897,8 @@ 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);
> + reply = ovs_flow_cmd_build_info(flow, dp, info,
> + OVS_FLOW_CMD_NEW, false);
>
> /* Clear stats. */
> if (a[OVS_FLOW_ATTR_CLEAR])
> @@ -885,11 +906,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:
> @@ -936,7 +960,7 @@ 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, dp, info, OVS_FLOW_CMD_NEW,
> true);
> if (IS_ERR(reply)) {
> err = PTR_ERR(reply);
> goto unlock;
> @@ -983,22 +1007,26 @@ 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;
> + reply = ovs_flow_cmd_alloc_info(flow, info, false);
> + if (IS_ERR(reply)) {
> + err = PTR_ERR(reply);
> 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@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
