[ovs-dev] [PATCH v6 6/6] datapath: Minimize ovs_flow_cmd_new|set critical sections.

[Jarno Rajahalme]

Thanks for the reviews!

All six pushed to master,

  Jarno

On Apr 1, 2014, at 3:20 PM, Pravin Shelar <pshelar at nicira.com> wrote:

> On Mon, Mar 31, 2014 at 11:18 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
...
>> @@ -871,40 +885,45 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
>>                 * request.  We also accept NLM_F_EXCL in case that bug ever
>>                 * gets fixed.
>>                 */
>> -               error = -EEXIST;
>> -               if (info->nlhdr->nlmsg_flags & (NLM_F_CREATE | NLM_F_EXCL))
>> +               if (unlikely(info->nlhdr->nlmsg_flags & (NLM_F_CREATE
>> +                                                        | NLM_F_EXCL))) {
>> +                       error = -EEXIST;
>>                        goto err_unlock_ovs;
>> -
>> +               }
>>                /* The unmasked key has to be the same for flow updates. */
>> -               if (!ovs_flow_cmp_unmasked_key(flow, &match))
>> +               if (unlikely(!ovs_flow_cmp_unmasked_key(flow, &match))) {
>> +                       error = -EEXIST;
>>                        goto err_unlock_ovs;
>> -
>> +               }
>>                /* Update actions. */
>>                old_acts = ovsl_dereference(flow->sf_acts);
>>                rcu_assign_pointer(flow->sf_acts, acts);
>> -               ovs_nla_free_flow_actions(old_acts);
>> -       }
>> 
>> -       reply = ovs_flow_cmd_build_info(flow, ovs_header->dp_ifindex,
>> -                                       info, OVS_FLOW_CMD_NEW, false);
>> -       ovs_unlock();
>> +               if (unlikely(reply)) {
>> +                       error = ovs_flow_cmd_fill_info(flow,
>> +                                                      ovs_header->dp_ifindex,
>> +                                                      reply, info->snd_portid,
>> +                                                      info->snd_seq, 0,
>> +                                                      OVS_FLOW_CMD_NEW);
>> +                       BUG_ON(error < 0);
>> +               }
>> +               ovs_unlock();
>> 
>> -       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));
>> +               ovs_nla_free_flow_actions(old_acts);
>> +               ovs_flow_free(new_flow, false);
>>        }
> How much do we save by unlocking ovs-lock before free, I think this is
> rare case. If so then we can move ovs-unlock out of if and else block.
> It is just easy to read.

NEW on an existing flow can happen if a second miss for the same flow appears before the new flow is set up. I do not know how often that happens, though.

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