On Thu, 17 May 2018 18:17:12 -0600
David Ahern <dsah...@gmail.com> wrote:
> On 5/17/18 4:36 PM, Stephen Hemminger wrote:
> > On Thu, 17 May 2018 16:22:37 -0600
> > dsah...@kernel.org wrote:
> >
> >> From: David Ahern <dsah...@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 <dsah...@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;
