[PATCH net-next-2.6] rtnetlink: rtnl_setlink() and rtnl_getlink() changes

Stephen, do you think we could change "ip link show dev ethX" to
let it use rtnl_getlink() instead of rtnl_dump_ifinfo() ?

Thanks !

[PATCH net-next-2.6]rtnetlink: rtnl_setlink() and rtnl_getlink() changes

rtnl_getlink() & rtnl_setlink() run with RTNL held, we can use
__dev_get_by_index() and __dev_get_by_name() variants and avoid
dev_hold()/dev_put()

Adds to rtnl_getlink() the capability to find a device by its name,
not only by its index.

Signed-off-by: Eric Dumazet <redacted>
---
 net/core/rtnetlink.c |   38 +++++++++++++++++++-------------------
 1 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index eb42873..ba13b09 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c

@@ -910,9 +910,9 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
 	err = -EINVAL;
 	ifm = nlmsg_data(nlh);
 	if (ifm->ifi_index > 0)
-		dev = dev_get_by_index(net, ifm->ifi_index);
+		dev = __dev_get_by_index(net, ifm->ifi_index);
 	else if (tb[IFLA_IFNAME])
-		dev = dev_get_by_name(net, ifname);
+		dev = __dev_get_by_name(net, ifname);
 	else
 		goto errout;
 

@@ -922,11 +922,9 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
 	}
 
 	if ((err = validate_linkmsg(dev, tb)) < 0)
-		goto errout_dev;
+		goto errout;
 
 	err = do_setlink(dev, ifm, tb, ifname, 0);
-errout_dev:
-	dev_put(dev);
 errout:
 	return err;
 }

@@ -1154,6 +1152,7 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr* nlh, void *arg)
 {
 	struct net *net = sock_net(skb->sk);
 	struct ifinfomsg *ifm;
+	char ifname[IFNAMSIZ];
 	struct nlattr *tb[IFLA_MAX+1];
 	struct net_device *dev = NULL;
 	struct sk_buff *nskb;

@@ -1163,19 +1162,23 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr* nlh, void *arg)
 	if (err < 0)
 		return err;
 
+	if (tb[IFLA_IFNAME])
+		nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
+
 	ifm = nlmsg_data(nlh);
-	if (ifm->ifi_index > 0) {
-		dev = dev_get_by_index(net, ifm->ifi_index);
-		if (dev == NULL)
-			return -ENODEV;
-	} else
+	if (ifm->ifi_index > 0)
+		dev = __dev_get_by_index(net, ifm->ifi_index);
+	else if (tb[IFLA_IFNAME])
+		dev = __dev_get_by_name(net, ifname);
+	else
 		return -EINVAL;
 
+	if (dev == NULL)
+		return -ENODEV;
+
 	nskb = nlmsg_new(if_nlmsg_size(dev), GFP_KERNEL);
-	if (nskb == NULL) {
-		err = -ENOBUFS;
-		goto errout;
-	}
+	if (nskb == NULL)
+		return -ENOBUFS;
 
 	err = rtnl_fill_ifinfo(nskb, dev, RTM_NEWLINK, NETLINK_CB(skb).pid,
 			       nlh->nlmsg_seq, 0, 0);

@@ -1183,11 +1186,8 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr* nlh, void *arg)
 		/* -EMSGSIZE implies BUG in if_nlmsg_size */
 		WARN_ON(err == -EMSGSIZE);
 		kfree_skb(nskb);
-		goto errout;
-	}
-	err = rtnl_unicast(nskb, net, NETLINK_CB(skb).pid);
-errout:
-	dev_put(dev);
+	} else
+		err = rtnl_unicast(nskb, net, NETLINK_CB(skb).pid);
 
 	return err;
 }

On Wed, 21 Oct 2009 22:59:31 +0200
Eric Dumazet [off-list ref] wrote:

Would work, but not sure what it gains.

Stephen Hemminger a écrit :

On Wed, 21 Oct 2009 22:59:31 +0200
Eric Dumazet [off-list ref] wrote:

quoted

Stephen, do you think we could change "ip link show dev ethX" to
let it use rtnl_getlink() instead of rtnl_dump_ifinfo() ?

Would work, but not sure what it gains.

It takes about one second to dump the table when we have 25.000 devices

Eric Dumazet a écrit :

Stephen Hemminger a écrit :

quoted

On Wed, 21 Oct 2009 22:59:31 +0200
Eric Dumazet [off-list ref] wrote:

quoted

Stephen, do you think we could change "ip link show dev ethX" to
let it use rtnl_getlink() instead of rtnl_dump_ifinfo() ?

Would work, but not sure what it gains.

It takes about one second to dump the table when we have 25.000 devices

Adding new links takes also lot of time in rtnl_dump_ifinfo(), we could
optimize it using a 256 fanout (using the ifindex hash table instead
of the single list)

But IMHO rtnl_dump_ifinfo() should be used only when needed, not when
querying/adding a particular device.

------------------------------------------------------------------------------
   PerfTop:   42745 irqs/sec  kernel:88.0% [100000 cycles],  (all, 8 CPUs)
------------------------------------------------------------------------------

             samples    pcnt   kernel function
             _______   _____   _______________

           231146.00 - 52.4% : rtnl_dump_ifinfo
            18491.00 -  4.2% : __register_sysctl_paths
            17700.00 -  4.0% : mwait_idle
            12883.00 -  2.9% : rtnl_fill_ifinfo
            12661.00 -  2.9% : schedule
             6324.00 -  1.4% : find_busiest_group
             5911.00 -  1.3% : _spin_lock_irqsave
             4862.00 -  1.1% : dev_get_stats
             4726.00 -  1.1% : copy_to_user
             4547.00 -  1.0% : __nla_put
             4117.00 -  0.9% : sysfs_find_dirent
             4090.00 -  0.9% : sysenter_past_esp
             3789.00 -  0.9% : fput
             3735.00 -  0.8% : __nla_reserve
             3699.00 -  0.8% : read_tsc

From: Eric Dumazet <redacted>
Date: Wed, 21 Oct 2009 22:59:31 +0200

rtnl_getlink() & rtnl_setlink() run with RTNL held, we can use
__dev_get_by_index() and __dev_get_by_name() variants and avoid
dev_hold()/dev_put()

Adds to rtnl_getlink() the capability to find a device by its name,
not only by its index.

Signed-off-by: Eric Dumazet <redacted>

Looks good, applied, thanks.