* [PATCH iproute2] ip link: Do not call ll_name_to_index when creating a new link @ 2018-05-17 22:22 dsahern 2018-05-17 22:36 ` Stephen Hemminger 0 siblings, 1 reply; 7+ messages in thread From: dsahern @ 2018-05-17 22:22 UTC (permalink / raw) To: netdev, stephen; +Cc: David Ahern From: David Ahern <dsahern@gmail.com> 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 <dsahern@gmail.com> --- 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(-) diff --git a/ip/ip_common.h b/ip/ip_common.h index 1b89795caa58..67f413474631 100644 --- a/ip/ip_common.h +++ b/ip/ip_common.h @@ -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); diff --git a/ip/iplink.c b/ip/iplink.c index e6bb4493120e..c8bf49ed3d24 100644 --- a/ip/iplink.c +++ b/ip/iplink.c @@ -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) diff --git a/ip/iplink_vxcan.c b/ip/iplink_vxcan.c index 8b08c9a70c65..e30a784d9851 100644 --- a/ip/iplink_vxcan.c +++ b/ip/iplink_vxcan.c @@ -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; diff --git a/ip/link_veth.c b/ip/link_veth.c index 33e8f2b102e7..68823931c0ec 100644 --- a/ip/link_veth.c +++ b/ip/link_veth.c @@ -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; -- 2.11.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] ip link: Do not call ll_name_to_index when creating a new link 2018-05-17 22:22 [PATCH iproute2] ip link: Do not call ll_name_to_index when creating a new link dsahern @ 2018-05-17 22:36 ` Stephen Hemminger 2018-05-17 22:47 ` David Ahern 2018-05-18 0:17 ` David Ahern 0 siblings, 2 replies; 7+ messages in thread From: Stephen Hemminger @ 2018-05-17 22:36 UTC (permalink / raw) To: dsahern; +Cc: netdev, David Ahern On Thu, 17 May 2018 16:22:37 -0600 dsahern@kernel.org wrote: > From: David Ahern <dsahern@gmail.com> > > 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 <dsahern@gmail.com> 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] ip link: Do not call ll_name_to_index when creating a new link 2018-05-17 22:36 ` Stephen Hemminger @ 2018-05-17 22:47 ` David Ahern 2018-05-18 0:17 ` David Ahern 1 sibling, 0 replies; 7+ messages in thread From: David Ahern @ 2018-05-17 22:47 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev On 5/17/18 4:36 PM, Stephen Hemminger wrote: > On Thu, 17 May 2018 16:22:37 -0600 > dsahern@kernel.org wrote: > >> From: David Ahern <dsahern@gmail.com> >> >> 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 <dsahern@gmail.com> > > 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] ip link: Do not call ll_name_to_index when creating a new link 2018-05-17 22:36 ` Stephen Hemminger 2018-05-17 22:47 ` David Ahern @ 2018-05-18 0:17 ` David Ahern 2018-05-18 22:08 ` Stephen Hemminger 1 sibling, 1 reply; 7+ messages in thread From: David Ahern @ 2018-05-18 0:17 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev On 5/17/18 4:36 PM, Stephen Hemminger wrote: > On Thu, 17 May 2018 16:22:37 -0600 > dsahern@kernel.org wrote: > >> From: David Ahern <dsahern@gmail.com> >> >> 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 <dsahern@gmail.com> > > 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] ip link: Do not call ll_name_to_index when creating a new link 2018-05-18 0:17 ` David Ahern @ 2018-05-18 22:08 ` Stephen Hemminger 2018-05-18 23:40 ` David Ahern 0 siblings, 1 reply; 7+ messages in thread From: Stephen Hemminger @ 2018-05-18 22:08 UTC (permalink / raw) To: David Ahern; +Cc: netdev On Thu, 17 May 2018 18:17:12 -0600 David Ahern <dsahern@gmail.com> wrote: > On 5/17/18 4:36 PM, Stephen Hemminger wrote: > > On Thu, 17 May 2018 16:22:37 -0600 > > dsahern@kernel.org wrote: > > > >> From: David Ahern <dsahern@gmail.com> > >> > >> 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 <dsahern@gmail.com> > > > > 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? diff --git a/ip/ip_common.h b/ip/ip_common.h index 1b89795caa58..49eb7d7bed40 100644 --- a/ip/ip_common.h +++ b/ip/ip_common.h @@ -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); diff --git a/ip/ipaddress.c b/ip/ipaddress.c index 75539e057f6a..00da14c6f97c 100644 --- a/ip/ipaddress.c +++ b/ip/ipaddress.c @@ -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"); diff --git a/ip/iplink.c b/ip/iplink.c index 22afe0221f3c..9ff5f692a1d4 100644 --- a/ip/iplink.c +++ b/ip/iplink.c @@ -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, diff --git a/ip/iplink_xdp.c b/ip/iplink_xdp.c index 83826358aa22..dd4fd1fd3a3b 100644 --- a/ip/iplink_xdp.c +++ b/ip/iplink_xdp.c @@ -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; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] ip link: Do not call ll_name_to_index when creating a new link 2018-05-18 22:08 ` Stephen Hemminger @ 2018-05-18 23:40 ` David Ahern 2018-05-25 15:21 ` Stephen Hemminger 0 siblings, 1 reply; 7+ messages in thread From: David Ahern @ 2018-05-18 23:40 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev On 5/18/18 4:08 PM, Stephen Hemminger wrote: > > 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH iproute2] ip link: Do not call ll_name_to_index when creating a new link 2018-05-18 23:40 ` David Ahern @ 2018-05-25 15:21 ` Stephen Hemminger 0 siblings, 0 replies; 7+ messages in thread From: Stephen Hemminger @ 2018-05-25 15:21 UTC (permalink / raw) To: David Ahern; +Cc: netdev On Fri, 18 May 2018 17:40:05 -0600 David Ahern <dsahern@gmail.com> wrote: > On 5/18/18 4:08 PM, Stephen Hemminger wrote: > > > > 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). ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-07 2:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-05-17 22:22 [PATCH iproute2] ip link: Do not call ll_name_to_index when creating a new link dsahern 2018-05-17 22:36 ` Stephen Hemminger 2018-05-17 22:47 ` David Ahern 2018-05-18 0:17 ` David Ahern 2018-05-18 22:08 ` Stephen Hemminger 2018-05-18 23:40 ` David Ahern 2018-05-25 15:21 ` Stephen Hemminger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).