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
