Thanks for the review, pushed to master,
Jarno
On Mar 28, 2014, at 1:25 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> 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
