Pushed to master with the function name change (ovs_vport_cmd_alloc() ->
ovs_vport_cmd_alloc_info()).
Jarno
On Mar 28, 2014, at 1:26 PM, Pravin Shelar <pshe...@nicira.com> wrote:
> On Tue, Mar 25, 2014 at 2:35 PM, Jarno Rajahalme <jrajaha...@nicira.com>
> wrote:
>> Move most memory allocations away from the ovs_mutex critical
>> sections. vport allocations still happen while the lock is taken, as
>> changing that would require major refactoring. Also, vports are
>> created very rarely so it should not matter.
>>
>> Change ovs_dp_cmd_get() now only takes the rcu_read_lock(), rather
>> than ovs_lock(), as nothing need to be changed. This was done by
>> ovs_vport_cmd_get() already.
>>
>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>> ---
>> v5: Rebase.
>>
>> datapath/datapath.c | 223
>> ++++++++++++++++++++++++++-------------------------
>> 1 file changed, 112 insertions(+), 111 deletions(-)
>>
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index aa3f789..ca18879 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -1271,23 +1271,9 @@ error:
>> return -EMSGSIZE;
>> }
>>
>> -/* Must be called with ovs_mutex. */
>> -static struct sk_buff *ovs_dp_cmd_build_info(struct datapath *dp,
>> - struct genl_info *info, u8 cmd)
>> +static struct sk_buff *ovs_dp_cmd_alloc_info(struct genl_info *info)
>> {
>> - struct sk_buff *skb;
>> - int retval;
>> -
>> - skb = genlmsg_new_unicast(ovs_dp_cmd_msg_size(), info, GFP_KERNEL);
>> - if (!skb)
>> - return ERR_PTR(-ENOMEM);
>> -
>> - retval = ovs_dp_cmd_fill_info(dp, skb, info->snd_portid,
>> info->snd_seq, 0, cmd);
>> - if (retval < 0) {
>> - kfree_skb(skb);
>> - return ERR_PTR(retval);
>> - }
>> - return skb;
>> + return genlmsg_new_unicast(ovs_dp_cmd_msg_size(), info, GFP_KERNEL);
>> }
>>
>> /* Called with rcu_read_lock or ovs_mutex. */
>> @@ -1340,12 +1326,14 @@ static int ovs_dp_cmd_new(struct sk_buff *skb,
>> struct genl_info *info)
>> if (!a[OVS_DP_ATTR_NAME] || !a[OVS_DP_ATTR_UPCALL_PID])
>> goto err;
>>
>> - ovs_lock();
>> + reply = ovs_dp_cmd_alloc_info(info);
>> + if (!reply)
>> + return -ENOMEM;
>>
>> err = -ENOMEM;
>> dp = kzalloc(sizeof(*dp), GFP_KERNEL);
>> if (dp == NULL)
>> - goto err_unlock_ovs;
>> + goto err_free_reply;
>>
>> ovs_dp_set_net(dp, hold_net(sock_net(skb->sk)));
>>
>> @@ -1380,6 +1368,9 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct
>> genl_info *info)
>>
>> ovs_dp_change(dp, a);
>>
>> + /* So far only local changes have been made, now need the lock. */
>> + ovs_lock();
>> +
>> vport = new_vport(&parms);
>> if (IS_ERR(vport)) {
>> err = PTR_ERR(vport);
>> @@ -1398,10 +1389,9 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct
>> genl_info *info)
>> goto err_destroy_ports_array;
>> }
>>
>> - reply = ovs_dp_cmd_build_info(dp, info, OVS_DP_CMD_NEW);
>> - err = PTR_ERR(reply);
>> - if (IS_ERR(reply))
>> - goto err_destroy_local_port;
>> + err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
>> + info->snd_seq, 0, OVS_DP_CMD_NEW);
>> + BUG_ON(err < 0);
>>
>> ovs_net = net_generic(ovs_dp_get_net(dp), ovs_net_id);
>> list_add_tail_rcu(&dp->list_node, &ovs_net->dps);
>> @@ -1411,9 +1401,8 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct
>> genl_info *info)
>> ovs_notify(reply, info, &ovs_dp_datapath_multicast_group);
>> return 0;
>>
>> -err_destroy_local_port:
>> - ovs_dp_detach_port(ovs_vport_ovsl(dp, OVSP_LOCAL));
>> err_destroy_ports_array:
>> + ovs_unlock();
>> kfree(dp->ports);
>> err_destroy_percpu:
>> free_percpu(dp->stats_percpu);
>> @@ -1422,8 +1411,8 @@ err_destroy_table:
>> err_free_dp:
>> release_net(ovs_dp_get_net(dp));
>> kfree(dp);
>> -err_unlock_ovs:
>> - ovs_unlock();
>> +err_free_reply:
>> + kfree_skb(reply);
>> err:
>> return err;
>> }
>> @@ -1461,25 +1450,29 @@ static int ovs_dp_cmd_del(struct sk_buff *skb,
>> struct genl_info *info)
>> struct datapath *dp;
>> int err;
>>
>> + reply = ovs_dp_cmd_alloc_info(info);
>> + if (!reply)
>> + return -ENOMEM;
>> +
>> ovs_lock();
>> dp = lookup_datapath(sock_net(skb->sk), info->userhdr, info->attrs);
>> err = PTR_ERR(dp);
>> if (IS_ERR(dp))
>> - goto unlock;
>> + goto err_unlock_free;
>>
>> - reply = ovs_dp_cmd_build_info(dp, info, OVS_DP_CMD_DEL);
>> - err = PTR_ERR(reply);
>> - if (IS_ERR(reply))
>> - goto unlock;
>> + err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
>> + info->snd_seq, 0, OVS_DP_CMD_DEL);
>> + BUG_ON(err < 0);
>>
>> __dp_destroy(dp);
>> - ovs_unlock();
>>
>> + ovs_unlock();
>> ovs_notify(reply, info, &ovs_dp_datapath_multicast_group);
>> -
>> return 0;
>> -unlock:
>> +
>> +err_unlock_free:
>> ovs_unlock();
>> + kfree_skb(reply);
>> return err;
>> }
>>
>> @@ -1489,29 +1482,29 @@ static int ovs_dp_cmd_set(struct sk_buff *skb,
>> struct genl_info *info)
>> struct datapath *dp;
>> int err;
>>
>> + reply = ovs_dp_cmd_alloc_info(info);
>> + if (!reply)
>> + return -ENOMEM;
>> +
>> ovs_lock();
>> dp = lookup_datapath(sock_net(skb->sk), info->userhdr, info->attrs);
>> err = PTR_ERR(dp);
>> if (IS_ERR(dp))
>> - goto unlock;
>> + goto err_unlock_free;
>>
>> ovs_dp_change(dp, info->attrs);
>>
>> - reply = ovs_dp_cmd_build_info(dp, info, OVS_DP_CMD_NEW);
>> - if (IS_ERR(reply)) {
>> - err = PTR_ERR(reply);
>> - netlink_set_err(sock_net(skb->sk)->genl_sock, 0,
>> - ovs_dp_datapath_multicast_group.id, err);
>> - err = 0;
>> - goto unlock;
>> - }
>> + err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
>> + info->snd_seq, 0, OVS_DP_CMD_NEW);
>> + BUG_ON(err < 0);
>>
>> ovs_unlock();
>> ovs_notify(reply, info, &ovs_dp_datapath_multicast_group);
>> -
>> return 0;
>> -unlock:
>> +
>> +err_unlock_free:
>> ovs_unlock();
>> + kfree_skb(reply);
>> return err;
>> }
>>
>> @@ -1521,24 +1514,26 @@ static int ovs_dp_cmd_get(struct sk_buff *skb,
>> struct genl_info *info)
>> struct datapath *dp;
>> int err;
>>
>> - ovs_lock();
>> + reply = ovs_dp_cmd_alloc_info(info);
>> + if (!reply)
>> + return -ENOMEM;
>> +
>> + rcu_read_lock();
>> dp = lookup_datapath(sock_net(skb->sk), info->userhdr, info->attrs);
>> if (IS_ERR(dp)) {
>> err = PTR_ERR(dp);
>> - goto unlock;
>> - }
>> -
>> - reply = ovs_dp_cmd_build_info(dp, info, OVS_DP_CMD_NEW);
>> - if (IS_ERR(reply)) {
>> - err = PTR_ERR(reply);
>> - goto unlock;
>> + goto err_unlock_free;
>> }
>> + err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
>> + info->snd_seq, 0, OVS_DP_CMD_NEW);
>> + BUG_ON(err < 0);
>> + rcu_read_unlock();
>>
>> - ovs_unlock();
>> return genlmsg_reply(reply, info);
>>
>> -unlock:
>> - ovs_unlock();
>> +err_unlock_free:
>> + rcu_read_unlock();
>> + kfree_skb(reply);
>> return err;
>> }
>>
>> @@ -1651,7 +1646,12 @@ error:
>> return err;
>> }
>>
>> -/* Called with ovs_mutex or RCU read lock. */
>> +static struct sk_buff *ovs_vport_cmd_alloc(void)
>> +{
>> + return nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>> +}
>> +
> ovs_vport_cmd_alloc is not clear, may be we can use _info suffix?
>
>> +/* Called with ovs_mutex, only via ovs_dp_notify_wq(). */
>> struct sk_buff *ovs_vport_cmd_build_info(struct vport *vport, u32 portid,
>> u32 seq, u8 cmd)
>> {
>> @@ -1713,33 +1713,35 @@ static int ovs_vport_cmd_new(struct sk_buff *skb,
>> struct genl_info *info)
>> u32 port_no;
>> int err;
>>
>> - err = -EINVAL;
>> if (!a[OVS_VPORT_ATTR_NAME] || !a[OVS_VPORT_ATTR_TYPE] ||
>> !a[OVS_VPORT_ATTR_UPCALL_PID])
>> - goto exit;
>> + return -EINVAL;
>> +
>> + port_no = a[OVS_VPORT_ATTR_PORT_NO]
>> + ? nla_get_u32(a[OVS_VPORT_ATTR_PORT_NO]) : 0;
>> + if (port_no >= DP_MAX_PORTS)
>> + return -EFBIG;
>> +
>> + reply = ovs_vport_cmd_alloc();
>> + if (!reply)
>> + return -ENOMEM;
>>
>> ovs_lock();
>> dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>> err = -ENODEV;
>> if (!dp)
>> - goto exit_unlock;
>> -
>> - if (a[OVS_VPORT_ATTR_PORT_NO]) {
>> - port_no = nla_get_u32(a[OVS_VPORT_ATTR_PORT_NO]);
>> -
>> - err = -EFBIG;
>> - if (port_no >= DP_MAX_PORTS)
>> - goto exit_unlock;
>> + goto exit_unlock_free;
>>
>> + if (port_no) {
>> vport = ovs_vport_ovsl(dp, port_no);
>> err = -EBUSY;
>> if (vport)
>> - goto exit_unlock;
>> + goto exit_unlock_free;
>> } else {
>> for (port_no = 1; ; port_no++) {
>> if (port_no >= DP_MAX_PORTS) {
>> err = -EFBIG;
>> - goto exit_unlock;
>> + goto exit_unlock_free;
>> }
>> vport = ovs_vport_ovsl(dp, port_no);
>> if (!vport)
>> @@ -1757,25 +1759,23 @@ static int ovs_vport_cmd_new(struct sk_buff *skb,
>> struct genl_info *info)
>> vport = new_vport(&parms);
>> err = PTR_ERR(vport);
>> if (IS_ERR(vport))
>> - goto exit_unlock;
>> + goto exit_unlock_free;
>>
>> err = 0;
>> if (a[OVS_VPORT_ATTR_STATS])
>> ovs_vport_set_stats(vport, nla_data(a[OVS_VPORT_ATTR_STATS]));
>>
>> - reply = ovs_vport_cmd_build_info(vport, info->snd_portid,
>> info->snd_seq,
>> - OVS_VPORT_CMD_NEW);
>> - if (IS_ERR(reply)) {
>> - err = PTR_ERR(reply);
>> - ovs_dp_detach_port(vport);
>> - goto exit_unlock;
>> - }
>> + err = ovs_vport_cmd_fill_info(vport, reply, info->snd_portid,
>> + info->snd_seq, 0, OVS_VPORT_CMD_NEW);
>> + BUG_ON(err < 0);
>> + ovs_unlock();
>>
>> ovs_notify(reply, info, &ovs_dp_vport_multicast_group);
>> + return 0;
>>
>> -exit_unlock:
>> +exit_unlock_free:
>> ovs_unlock();
>> -exit:
>> + kfree_skb(reply);
>> return err;
>> }
>>
>> @@ -1786,28 +1786,26 @@ static int ovs_vport_cmd_set(struct sk_buff *skb,
>> struct genl_info *info)
>> struct vport *vport;
>> int err;
>>
>> + reply = ovs_vport_cmd_alloc();
>> + if (!reply)
>> + return -ENOMEM;
>> +
>> ovs_lock();
>> vport = lookup_vport(sock_net(skb->sk), info->userhdr, a);
>> err = PTR_ERR(vport);
>> if (IS_ERR(vport))
>> - goto exit_unlock;
>> + goto exit_unlock_free;
>>
>> if (a[OVS_VPORT_ATTR_TYPE] &&
>> nla_get_u32(a[OVS_VPORT_ATTR_TYPE]) != vport->ops->type) {
>> err = -EINVAL;
>> - goto exit_unlock;
>> - }
>> -
>> - reply = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>> - if (!reply) {
>> - err = -ENOMEM;
>> - goto exit_unlock;
>> + goto exit_unlock_free;
>> }
>>
>> if (a[OVS_VPORT_ATTR_OPTIONS]) {
>> err = ovs_vport_set_options(vport, a[OVS_VPORT_ATTR_OPTIONS]);
>> if (err)
>> - goto exit_free;
>> + goto exit_unlock_free;
>> }
>>
>> if (a[OVS_VPORT_ATTR_STATS])
>> @@ -1819,15 +1817,14 @@ static int ovs_vport_cmd_set(struct sk_buff *skb,
>> struct genl_info *info)
>> err = ovs_vport_cmd_fill_info(vport, reply, info->snd_portid,
>> info->snd_seq, 0, OVS_VPORT_CMD_NEW);
>> BUG_ON(err < 0);
>> -
>> ovs_unlock();
>> +
>> ovs_notify(reply, info, &ovs_dp_vport_multicast_group);
>> return 0;
>>
>> -exit_free:
>> - kfree_skb(reply);
>> -exit_unlock:
>> +exit_unlock_free:
>> ovs_unlock();
>> + kfree_skb(reply);
>> return err;
>> }
>>
>> @@ -1838,30 +1835,33 @@ static int ovs_vport_cmd_del(struct sk_buff *skb,
>> struct genl_info *info)
>> struct vport *vport;
>> int err;
>>
>> + reply = ovs_vport_cmd_alloc();
>> + if (!reply)
>> + return -ENOMEM;
>> +
>> ovs_lock();
>> vport = lookup_vport(sock_net(skb->sk), info->userhdr, a);
>> err = PTR_ERR(vport);
>> if (IS_ERR(vport))
>> - goto exit_unlock;
>> + goto exit_unlock_free;
>>
>> if (vport->port_no == OVSP_LOCAL) {
>> err = -EINVAL;
>> - goto exit_unlock;
>> + goto exit_unlock_free;
>> }
>>
>> - reply = ovs_vport_cmd_build_info(vport, info->snd_portid,
>> - info->snd_seq, OVS_VPORT_CMD_DEL);
>> - err = PTR_ERR(reply);
>> - if (IS_ERR(reply))
>> - goto exit_unlock;
>> -
>> - err = 0;
>> + err = ovs_vport_cmd_fill_info(vport, reply, info->snd_portid,
>> + info->snd_seq, 0, OVS_VPORT_CMD_DEL);
>> + BUG_ON(err < 0);
>> ovs_dp_detach_port(vport);
>> + ovs_unlock();
>>
>> ovs_notify(reply, info, &ovs_dp_vport_multicast_group);
>> + return 0;
>>
>> -exit_unlock:
>> +exit_unlock_free:
>> ovs_unlock();
>> + kfree_skb(reply);
>> return err;
>> }
>>
>> @@ -1873,24 +1873,25 @@ static int ovs_vport_cmd_get(struct sk_buff *skb,
>> struct genl_info *info)
>> struct vport *vport;
>> int err;
>>
>> + reply = ovs_vport_cmd_alloc();
>> + if (!reply)
>> + return -ENOMEM;
>> +
>> rcu_read_lock();
>> vport = lookup_vport(sock_net(skb->sk), ovs_header, a);
>> err = PTR_ERR(vport);
>> if (IS_ERR(vport))
>> - goto exit_unlock;
>> -
>> - reply = ovs_vport_cmd_build_info(vport, info->snd_portid,
>> - info->snd_seq, OVS_VPORT_CMD_NEW);
>> - err = PTR_ERR(reply);
>> - if (IS_ERR(reply))
>> - goto exit_unlock;
>> -
>> + goto exit_unlock_free;
>> + err = ovs_vport_cmd_fill_info(vport, reply, info->snd_portid,
>> + info->snd_seq, 0, OVS_VPORT_CMD_NEW);
>> + BUG_ON(err < 0);
>> rcu_read_unlock();
>>
>> return genlmsg_reply(reply, info);
>>
>> -exit_unlock:
>> +exit_unlock_free:
>> rcu_read_unlock();
>> + kfree_skb(reply);
>> return err;
>> }
>>
> Acked-by: Pravin B Shelar <pshe...@nicira.com>
>
>> --
>> 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
