ip link: Do not call ll_name_to_index when creating a new link

PATCH In review [email protected] 3 participants 7 messages May 17, 2018 – May 7, 2020

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

[email protected]

author

2018-05-17 22:22 UTC #0 lore raw

PATCH 1/1 ip link: Do not call ll_name_to_index when creating a new link

From: David Ahern <[email protected]>

Using iproute2 to create a bridge and add 4094 vlans to it can take from 2 to 3 *minutes*. The reason is the extraneous call to ll_name_to_index. ll_name_to_index results in an ioctl(SIOCGIFINDEX) call which in turn invokes dev_load. If the index does not exist, which it won't when creating a new link, dev_load calls modprobe twice -- once for netdev-NAME and again for NAME. This is unnecessary overhead for each link create.

When ip link is invoked for a new device, there is no reason to call ll_name_to_index for the new device. With this patch, creating a bridge and adding 4094 vlans takes less than 3 *seconds*.

Signed-off-by David Ahern <[email protected]>

---
 ip/ip_common.h    |  3 ++-
 ip/iplink.c       | 22 +++++++++++++---------
 ip/iplink_vxcan.c |  3 ++-
 ip/link_veth.c    |  3 ++-
 4 files changed, 19 insertions(+), 12 deletions(-)

ip/ip_common.h +2 -1

index 1b89795caa58..67f413474631 100644
@@ -132,7 +132,8 @@ struct link_util {
 
 struct link_util *get_link_kind(const char *kind);
 
-int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type);
+int iplink_parse(int argc, char **argv, struct iplink_req *req,
+		 char **type, bool is_add_cmd);
 
 /* iplink_bridge.c */
 void br_dump_bridge_id(const struct ifla_bridge_id *id, char *buf, size_t len);

ip/iplink.c +13 -9

index e6bb4493120e..c8bf49ed3d24 100644
@@ -571,7 +571,8 @@ static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 	return 0;
 }
 
-int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
+int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type,
+		 bool is_add_cmd)
 {
 	char *name = NULL;
 	char *dev = NULL;

@@ -610,7 +611,8 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 			name = *argv;
 			if (!dev) {
 				dev = name;
-				dev_index = ll_name_to_index(dev);
+				if (!is_add_cmd)
+					dev_index = ll_name_to_index(dev);
 			}
 		} else if (strcmp(*argv, "index") == 0) {
 			NEXT_ARG();

@@ -919,7 +921,8 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 			if (check_ifname(*argv))
 				invarg("\"dev\" not a valid ifname", *argv);
 			dev = *argv;
-			dev_index = ll_name_to_index(dev);
+			if (!is_add_cmd)
+				dev_index = ll_name_to_index(dev);
 		}
 		argc--; argv++;
 	}

@@ -1011,7 +1014,8 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 	return ret;
 }
 
-static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
+static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv,
+			 bool is_add_cmd)
 {
 	char *type = NULL;
 	struct iplink_req req = {

@@ -1022,7 +1026,7 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 	};
 	int ret;
 
-	ret = iplink_parse(argc, argv, &req, &type);
+	ret = iplink_parse(argc, argv, &req, &type, is_add_cmd);
 	if (ret < 0)
 		return ret;

@@ -1630,18 +1634,18 @@ int do_iplink(int argc, char **argv)
 		if (matches(*argv, "add") == 0)
 			return iplink_modify(RTM_NEWLINK,
 					     NLM_F_CREATE|NLM_F_EXCL,
-					     argc-1, argv+1);
+					     argc-1, argv+1, true);
 		if (matches(*argv, "set") == 0 ||
 		    matches(*argv, "change") == 0)
 			return iplink_modify(RTM_NEWLINK, 0,
-					     argc-1, argv+1);
+					     argc-1, argv+1, false);
 		if (matches(*argv, "replace") == 0)
 			return iplink_modify(RTM_NEWLINK,
 					     NLM_F_CREATE|NLM_F_REPLACE,
-					     argc-1, argv+1);
+					     argc-1, argv+1, false);
 		if (matches(*argv, "delete") == 0)
 			return iplink_modify(RTM_DELLINK, 0,
-					     argc-1, argv+1);
+					     argc-1, argv+1, false);
 	} else {
 #if IPLINK_IOCTL_COMPAT
 		if (matches(*argv, "set") == 0)

ip/iplink_vxcan.c +2 -1

index 8b08c9a70c65..e30a784d9851 100644
@@ -56,7 +56,8 @@ static int vxcan_parse_opt(struct link_util *lu, int argc, char **argv,
 
 	n->nlmsg_len += sizeof(struct ifinfomsg);
 
-	err = iplink_parse(argc - 1, argv + 1, (struct iplink_req *)n, &type);
+	err = iplink_parse(argc - 1, argv + 1, (struct iplink_req *)n, &type,
+			   false);
 	if (err < 0)
 		return err;

ip/link_veth.c +2 -1

index 33e8f2b102e7..68823931c0ec 100644
@@ -54,7 +54,8 @@ static int veth_parse_opt(struct link_util *lu, int argc, char **argv,
 
 	n->nlmsg_len += sizeof(struct ifinfomsg);
 
-	err = iplink_parse(argc - 1, argv + 1, (struct iplink_req *)n, &type);
+	err = iplink_parse(argc - 1, argv + 1, (struct iplink_req *)n, &type,
+			   false);
 	if (err < 0)
 		return err;

Signature · 1 line

2.11.0

Stephen Hemminger

2018-05-17 22:36 UTC #1 lore raw

Quoted 10 lines · On Thu, 17 May 2018 16:22:37 -0600 [email protected] wrote:

From: David Ahern <[email protected]>
Using iproute2 to create a bridge and add 4094 vlans to it can take from 2 to 3 *minutes*. The reason is the extraneous call to ll_name_to_index. ll_name_to_index results in an ioctl(SIOCGIFINDEX) call which in turn invokes dev_load. If the index does not exist, which it won't when creating a new link, dev_load calls modprobe twice -- once for netdev-NAME and again for NAME. This is unnecessary overhead for each link create.

Quoted 5 lines

When ip link is invoked for a new device, there is no reason to call ll_name_to_index for the new device. With this patch, creating a bridge and adding 4094 vlans takes less than 3 *seconds*.
Signed-off-by David Ahern <[email protected]>

Yes this looks like a real problem.
Isn't the cache supposed to reduce this?

Don't like to make lots of special case flags.

David Ahern

2018-05-18 00:17 UTC #2 lore raw

Quoted 17 lines · Stephen Hemminger

Quoted 14 lines · On Thu, 17 May 2018 16:22:37 -0600 [email protected] wrote:
From: David Ahern <[email protected]>
Using iproute2 to create a bridge and add 4094 vlans to it can take from 2 to 3 *minutes*. The reason is the extraneous call to ll_name_to_index. ll_name_to_index results in an ioctl(SIOCGIFINDEX) call which in turn invokes dev_load. If the index does not exist, which it won't when creating a new link, dev_load calls modprobe twice -- once for netdev-NAME and again for NAME. This is unnecessary overhead for each link create.
When ip link is invoked for a new device, there is no reason to call ll_name_to_index for the new device. With this patch, creating a bridge and adding 4094 vlans takes less than 3 *seconds*.

Quoted 7 lines

Quoted 1 line
Signed-off-by David Ahern <[email protected]>
Yes this looks like a real problem.
Isn't the cache supposed to reduce this?
Don't like to make lots of special case flags.

The device does not exist, so it won't be in any cache. ll_name_to_index already checks it though before calling if_nametoindex.

Stephen Hemminger

2018-05-18 22:08 UTC #3 lore raw

Quoted 20 lines · On Thu, 17 May 2018 18:17:12 -0600 David Ahern <[email protected]> wrote:

Quoted 19 lines · Stephen Hemminger
Quoted 15 lines · On Thu, 17 May 2018 16:22:37 -0600 [email protected] wrote:
From: David Ahern <[email protected]>
Using iproute2 to create a bridge and add 4094 vlans to it can take from 2 to 3 *minutes*. The reason is the extraneous call to ll_name_to_index. ll_name_to_index results in an ioctl(SIOCGIFINDEX) call which in turn invokes dev_load. If the index does not exist, which it won't when creating a new link, dev_load calls modprobe twice -- once for netdev-NAME and again for NAME. This is unnecessary overhead for each link create.
When ip link is invoked for a new device, there is no reason to call ll_name_to_index for the new device. With this patch, creating a bridge and adding 4094 vlans takes less than 3 *seconds*.
Signed-off-by David Ahern <[email protected]>

Quoted 8 lines

Quoted 5 lines
Yes this looks like a real problem.
Isn't the cache supposed to reduce this?
Don't like to make lots of special case flags.
The device does not exist, so it won't be in any cache. ll_name_to_index already checks it though before calling if_nametoindex.

Good point, I just don't like adding more conditional paths in a function it is a common source of errors.

What about just pushing the lookup down to the leaf functions that need it?

ip/ip_common.h +2 -2

index 1b89795caa58..49eb7d7bed40 100644
@@ -36,7 +36,7 @@ int print_addrlabel(const struct sockaddr_nl *who,
 int print_neigh(const struct sockaddr_nl *who,
 		struct nlmsghdr *n, void *arg);
 int ipaddr_list_link(int argc, char **argv);
-void ipaddr_get_vf_rate(int, int *, int *, int);
+void ipaddr_get_vf_rate(int, int *, int *, const char *);
 void iplink_usage(void) __attribute__((noreturn));
 
 void iproute_reset_filter(int ifindex);

@@ -145,7 +145,7 @@ int lwt_parse_encap(struct rtattr *rta, size_t len, int *argcp, char ***argvp);
 void lwt_print_encap(FILE *fp, struct rtattr *encap_type, struct rtattr *encap);
 
 /* iplink_xdp.c */
-int xdp_parse(int *argc, char ***argv, struct iplink_req *req, __u32 ifindex,
+int xdp_parse(int *argc, char ***argv, struct iplink_req *req, const char *ifname,
 	      bool generic, bool drv, bool offload);
 void xdp_dump(FILE *fp, struct rtattr *tb, bool link, bool details);

ip/ipaddress.c +8 -2

index 75539e057f6a..00da14c6f97c 100644
@@ -1967,14 +1967,20 @@ ipaddr_loop_each_vf(struct rtattr *tb[], int vfnum, int *min, int *max)
 	exit(1);
 }
 
-void ipaddr_get_vf_rate(int vfnum, int *min, int *max, int idx)
+void ipaddr_get_vf_rate(int vfnum, int *min, int *max, const char *dev)
 {
 	struct nlmsg_chain linfo = { NULL, NULL};
 	struct rtattr *tb[IFLA_MAX+1];
 	struct ifinfomsg *ifi;
 	struct nlmsg_list *l;
 	struct nlmsghdr *n;
-	int len;
+	int idx, len;
+
+	idx = ll_name_to_index(dev);
+	if (idx == 0) {
+		fprintf(stderr, "Device %s does not exist\n", dev);
+		exit(1);
+	}
 
 	if (rtnl_wilddump_request(&rth, AF_UNSPEC, RTM_GETLINK) < 0) {
 		perror("Cannot send dump request");

ip/iplink.c +14 ~~-14~~

index 22afe0221f3c..9ff5f692a1d4 100644
@@ -242,9 +242,10 @@ static int iplink_have_newlink(void)
 }
 #endif /* ! IPLINK_IOCTL_COMPAT */
 
-static int nl_get_ll_addr_len(unsigned int dev_index)
+static int nl_get_ll_addr_len(const char *ifname)
 {
 	int len;
+	int dev_index = ll_name_to_index(ifname);
 	struct iplink_req req = {
 		.n = {
 			.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),

@@ -259,6 +260,9 @@ static int nl_get_ll_addr_len(unsigned int dev_index)
 	struct nlmsghdr *answer;
 	struct rtattr *tb[IFLA_MAX+1];
 
+	if (dev_index == 0)
+		return -1;
+
 	if (rtnl_talk(&rth, &req.n, &answer) < 0)
 		return -1;

@@ -337,7 +341,7 @@ static void iplink_parse_vf_vlan_info(int vf, int *argcp, char ***argvp,
 }
 
 static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
-			   struct iplink_req *req, int dev_index)
+			   struct iplink_req *req, const char *dev)
 {
 	char new_rate_api = 0, count = 0, override_legacy_rate = 0;
 	struct ifla_vf_rate tivt;

@@ -373,7 +377,7 @@ static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 		NEXT_ARG();
 		if (matches(*argv, "mac") == 0) {
 			struct ifla_vf_mac ivm = { 0 };
-			int halen = nl_get_ll_addr_len(dev_index);
+			int halen = nl_get_ll_addr_len(dev);
 
 			NEXT_ARG();
 			ivm.vf = vf;

@@ -542,7 +546,7 @@ static int iplink_parse_vf(int vf, int *argcp, char ***argvp,
 		int tmin, tmax;
 
 		if (tivt.min_tx_rate == -1 || tivt.max_tx_rate == -1) {
-			ipaddr_get_vf_rate(tivt.vf, &tmin, &tmax, dev_index);
+			ipaddr_get_vf_rate(tivt.vf, &tmin, &tmax, dev);
 			if (tivt.min_tx_rate == -1)
 				tivt.min_tx_rate = tmin;
 			if (tivt.max_tx_rate == -1)

@@ -583,7 +587,6 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 	int vf = -1;
 	int numtxqueues = -1;
 	int numrxqueues = -1;
-	int dev_index = 0;
 	int link_netnsid = -1;
 	int index = 0;
 	int group = -1;

@@ -605,10 +608,8 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 			if (check_ifname(*argv))
 				invarg("\"name\" not a valid ifname", *argv);
 			name = *argv;
-			if (!dev) {
+			if (!dev)
 				dev = name;
-				dev_index = ll_name_to_index(dev);
-			}
 		} else if (strcmp(*argv, "index") == 0) {
 			NEXT_ARG();
 			if (index)

@@ -660,7 +661,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 			bool offload = strcmp(*argv, "xdpoffload") == 0;
 
 			NEXT_ARG();
-			if (xdp_parse(&argc, &argv, req, dev_index,
+			if (xdp_parse(&argc, &argv, req, dev,
 				      generic, drv, offload))
 				exit(-1);

@@ -750,10 +751,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 
 			vflist = addattr_nest(&req->n, sizeof(*req),
 					      IFLA_VFINFO_LIST);
-			if (dev_index == 0)
+			if (!dev)
 				missarg("dev");
 
-			len = iplink_parse_vf(vf, &argc, &argv, req, dev_index);
+			len = iplink_parse_vf(vf, &argc, &argv, req, dev);
 			if (len < 0)
 				return -1;
 			addattr_nest_end(&req->n, vflist);

@@ -916,7 +917,6 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 			if (check_ifname(*argv))
 				invarg("\"dev\" not a valid ifname", *argv);
 			dev = *argv;
-			dev_index = ll_name_to_index(dev);
 		}
 		argc--; argv++;
 	}

@@ -931,8 +931,8 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 	else if (!strcmp(name, dev))
 		name = dev;
 
-	if (dev_index && addr_len) {
-		int halen = nl_get_ll_addr_len(dev_index);
+	if (dev && addr_len) {
+		int halen = nl_get_ll_addr_len(dev);
 
 		if (halen >= 0 && halen != addr_len) {
 			fprintf(stderr,

ip/iplink_xdp.c +4 -2

index 83826358aa22..dd4fd1fd3a3b 100644
@@ -48,8 +48,8 @@ static int xdp_delete(struct xdp_req *xdp)
 	return 0;
 }
 
-int xdp_parse(int *argc, char ***argv, struct iplink_req *req, __u32 ifindex,
-	      bool generic, bool drv, bool offload)
+int xdp_parse(int *argc, char ***argv, struct iplink_req *req,
+	      const char *ifname, bool generic, bool drv, bool offload)
 {
 	struct bpf_cfg_in cfg = {
 		.type = BPF_PROG_TYPE_XDP,

@@ -61,6 +61,8 @@ int xdp_parse(int *argc, char ***argv, struct iplink_req *req, __u32 ifindex,
 	};
 
 	if (offload) {
+		int ifindex = ll_name_to_index(ifname);
+
 		if (!ifindex)
 			incomplete_command();
 		cfg.ifindex = ifindex;

David Ahern

2018-05-18 23:40 UTC #4 lore raw

Quoted 3 lines · Stephen Hemminger

What about just pushing the lookup down to the leaf functions that need it?

That should work as well. You want to re-send a formal patch?

Stephen Hemminger

2018-05-25 15:21 UTC #5 lore raw

Quoted 7 lines · On Fri, 18 May 2018 17:40:05 -0600 David Ahern <[email protected]> wrote:

Quoted 3 lines · Stephen Hemminger
What about just pushing the lookup down to the leaf functions that need it?
That should work as well. You want to re-send a formal patch?

I just pushed it up as a formal patch (with your text).

David Ahern

2020-05-07 02:36 UTC #6 lore raw

In reply to Stephen Hemminger

Quoted 17 lines · Stephen Hemminger

Quoted 14 lines · On Thu, 17 May 2018 16:22:37 -0600 [email protected] wrote:
From: David Ahern <[email protected]>
Using iproute2 to create a bridge and add 4094 vlans to it can take from 2 to 3 *minutes*. The reason is the extraneous call to ll_name_to_index. ll_name_to_index results in an ioctl(SIOCGIFINDEX) call which in turn invokes dev_load. If the index does not exist, which it won't when creating a new link, dev_load calls modprobe twice -- once for netdev-NAME and again for NAME. This is unnecessary overhead for each link create.
When ip link is invoked for a new device, there is no reason to call ll_name_to_index for the new device. With this patch, creating a bridge and adding 4094 vlans takes less than 3 *seconds*.

Quoted 7 lines

Quoted 1 line
Signed-off-by David Ahern <[email protected]>
Yes this looks like a real problem.
Isn't the cache supposed to reduce this?
Don't like to make lots of special case flags.

The device does not exist, so it won't be in any cache. ll_name_to_index already checks it though before calling if_nametoindex.