[ovs-dev] [PATCH v5 1/9] datapath: Build flow cmd netlink reply only if needed.

Jarno Rajahalme jrajahalme at nicira.com
Sat Mar 29 22:55:47 UTC 2014


Thanks for the review, pushed to master,

  Jarno

On Mar 28, 2014, at 1:25 PM, Pravin Shelar <pshelar at nicira.com> wrote:

> On Tue, Mar 25, 2014 at 2:35 PM, 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.  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 <jrajahalme at nicira.com>
> 
> LGTM
> Acked-by: Pravin B Shelar <pshelar at 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 at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140329/b036b752/attachment-0005.html>


More information about the dev mailing list