rtnetlink: rtnl_setlink() and rtnl_getlink() changes

PATCH In review Eric Dumazet 3 participants 5 messages Oct 21, 2009 – Oct 22, 2009

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Eric Dumazet

author

2009-10-21 20:59 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---
 net/core/rtnetlink.c |   38 +++++++++++++++++++-------------------
 1 files changed, 19 insertions(+), 19 deletions(-)

net/core/rtnetlink.c +19 ~~-19~~

index eb42873..ba13b09 100644
@@ -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;
 }

Eric Dumazet

author

2009-10-22 02:59 UTC #2 lore raw

Quoted 8 lines · Stephen Hemminger

Quoted 3 lines · On Wed, 21 Oct 2009 22:59:31 +0200 Eric Dumazet <[email protected]> wrote:
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

author

2009-10-22 04:48 UTC #3 lore raw

Quoted 10 lines · Eric Dumazet

Quoted 7 lines · Stephen Hemminger
Quoted 3 lines · On Wed, 21 Oct 2009 22:59:31 +0200 Eric Dumazet <[email protected]> wrote:
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

David Miller

maintainer

2009-10-22 11:33 UTC #4 lore raw

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Wed, 21 Oct 2009 22:59:31 +0200

Quoted 8 lines

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 <[email protected]>

Looks good, applied, thanks.