| Message ID | 20190107204130.32144-1-dsahern@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next] Improve batch times by caching link lookups | expand |
On 01/07/2019 12:41 PM, David Ahern wrote: > From: David Ahern <dsahern@gmail.com> > > ip route uses ll_name_to_index to convert the user given device name to an > index. At the moment ll_name_to_index uses if_nametoindex which is ioctl > based and does not cache the result. When using a batch file this means > the same device lookups can be done repeatedly adding unnecessary overhead > (socket + ioctl call for each device lookup). > > Add a new function, ll_link_get, to send a netlink based RTM_GETLINK. If > successful, cache the result in idx_head and name_head so future lookups > can re-use the entry. > > With this change the time to install routes via a batch file is reduced > from 30.7 seconds to 17.6 seconds (720,022 routes with 2 ecmp nexthops > where the nexthop device is given). > What time increase if we have 10,000 devices, and install one route ? Caching 10,000 devices would be quite a waste.
On 1/7/19 1:57 PM, Eric Dumazet wrote: > > > On 01/07/2019 12:41 PM, David Ahern wrote: >> From: David Ahern <dsahern@gmail.com> >> >> ip route uses ll_name_to_index to convert the user given device name to an >> index. At the moment ll_name_to_index uses if_nametoindex which is ioctl >> based and does not cache the result. When using a batch file this means >> the same device lookups can be done repeatedly adding unnecessary overhead >> (socket + ioctl call for each device lookup). >> >> Add a new function, ll_link_get, to send a netlink based RTM_GETLINK. If >> successful, cache the result in idx_head and name_head so future lookups >> can re-use the entry. >> >> With this change the time to install routes via a batch file is reduced >> from 30.7 seconds to 17.6 seconds (720,022 routes with 2 ecmp nexthops >> where the nexthop device is given). >> > > What time increase if we have 10,000 devices, and install one route ? > > Caching 10,000 devices would be quite a waste. > It only lookups up the device if ll_name_to_index is invoked for the device. This is NOT a link dump and cache; it only adds lookups to the cache.
On 01/07/2019 12:58 PM, David Ahern wrote: > On 1/7/19 1:57 PM, Eric Dumazet wrote: >> >> >> On 01/07/2019 12:41 PM, David Ahern wrote: >>> From: David Ahern <dsahern@gmail.com> >>> >>> ip route uses ll_name_to_index to convert the user given device name to an >>> index. At the moment ll_name_to_index uses if_nametoindex which is ioctl >>> based and does not cache the result. When using a batch file this means >>> the same device lookups can be done repeatedly adding unnecessary overhead >>> (socket + ioctl call for each device lookup). >>> >>> Add a new function, ll_link_get, to send a netlink based RTM_GETLINK. If >>> successful, cache the result in idx_head and name_head so future lookups >>> can re-use the entry. >>> >>> With this change the time to install routes via a batch file is reduced >>> from 30.7 seconds to 17.6 seconds (720,022 routes with 2 ecmp nexthops >>> where the nexthop device is given). >>> >> >> What time increase if we have 10,000 devices, and install one route ? >> >> Caching 10,000 devices would be quite a waste. >> > > It only lookups up the device if ll_name_to_index is invoked for the > device. > > This is NOT a link dump and cache; it only adds lookups to the cache. > I see, maybe split this in two patches to avoid the confusion. Why is the RTNETLINK approach better than ioctl() ? If this is the case, maybe if_nametoindex() should be fixed, instead of carrying this locally in iproute2.
On 1/7/19 2:06 PM, Eric Dumazet wrote: > > > On 01/07/2019 12:58 PM, David Ahern wrote: >> On 1/7/19 1:57 PM, Eric Dumazet wrote: >>> >>> >>> On 01/07/2019 12:41 PM, David Ahern wrote: >>>> From: David Ahern <dsahern@gmail.com> >>>> >>>> ip route uses ll_name_to_index to convert the user given device name to an >>>> index. At the moment ll_name_to_index uses if_nametoindex which is ioctl >>>> based and does not cache the result. When using a batch file this means >>>> the same device lookups can be done repeatedly adding unnecessary overhead >>>> (socket + ioctl call for each device lookup). >>>> >>>> Add a new function, ll_link_get, to send a netlink based RTM_GETLINK. If >>>> successful, cache the result in idx_head and name_head so future lookups >>>> can re-use the entry. >>>> >>>> With this change the time to install routes via a batch file is reduced >>>> from 30.7 seconds to 17.6 seconds (720,022 routes with 2 ecmp nexthops >>>> where the nexthop device is given). >>>> >>> >>> What time increase if we have 10,000 devices, and install one route ? >>> >>> Caching 10,000 devices would be quite a waste. >>> >> >> It only lookups up the device if ll_name_to_index is invoked for the >> device. >> >> This is NOT a link dump and cache; it only adds lookups to the cache. >> > > I see, maybe split this in two patches to avoid the confusion. > > Why is the RTNETLINK approach better than ioctl() ? I could very well cache the if_nametoindex result but that is only name <--> idx. To avoid making assumptions on how the ll_map code is used in the future, RTM_GETLINK is used to leverage ll_remember_index and have the cache entries always be consistently filled. > > If this is the case, maybe if_nametoindex() should be fixed, instead of carrying this locally in iproute2. > if_nametoindex has to do socket() - ioctl() - close() for every device lookup. It's not a bug and caching the socket descriptor makes assumptions that libc should not be making.
On Mon, 7 Jan 2019 12:41:30 -0800 David Ahern <dsahern@kernel.org> wrote: > From: David Ahern <dsahern@gmail.com> > > ip route uses ll_name_to_index to convert the user given device name to an > index. At the moment ll_name_to_index uses if_nametoindex which is ioctl > based and does not cache the result. When using a batch file this means > the same device lookups can be done repeatedly adding unnecessary overhead > (socket + ioctl call for each device lookup). > > Add a new function, ll_link_get, to send a netlink based RTM_GETLINK. If > successful, cache the result in idx_head and name_head so future lookups > can re-use the entry. > > With this change the time to install routes via a batch file is reduced > from 30.7 seconds to 17.6 seconds (720,022 routes with 2 ecmp nexthops > where the nexthop device is given). > > Signed-off-by: David Ahern <dsahern@gmail.com> What if a ip command in the batch does a rename?
On 1/7/19 2:31 PM, Stephen Hemminger wrote: > > What if a ip command in the batch does a rename? > Simplest action (meaning least overhead for non-batch) is to drop any entry from the cache. If a later command needs it, the cache entry can be re-created.
diff --git a/lib/ll_map.c b/lib/ll_map.c index 1ab8ef0758ac..4fbd9fa70dd7 100644 --- a/lib/ll_map.c +++ b/lib/ll_map.c @@ -192,6 +192,46 @@ int ll_index_to_flags(unsigned idx) return im ? im->flags : -1; } +static int ll_link_get(const char *name) +{ + struct { + struct nlmsghdr n; + struct ifinfomsg ifm; + char buf[1024]; + } req = { + .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)), + .n.nlmsg_flags = NLM_F_REQUEST, + .n.nlmsg_type = RTM_GETLINK, + }; + __u32 filt_mask = RTEXT_FILTER_VF | RTEXT_FILTER_SKIP_STATS; + struct rtnl_handle rth = {}; + struct nlmsghdr *answer; + int rc = 0; + + if (rtnl_open(&rth, 0) < 0) + return 0; + + addattr32(&req.n, sizeof(req), IFLA_EXT_MASK, filt_mask); + addattr_l(&req.n, sizeof(req), IFLA_IFNAME, name, + strlen(name) + 1); + + if (rtnl_talk(&rth, &req.n, &answer) < 0) + goto out; + + /* add entry to cache */ + rc = ll_remember_index(answer, NULL); + if (!rc) { + struct ifinfomsg *ifm = NLMSG_DATA(answer); + + rc = ifm->ifi_index; + } + + free(answer); +out: + rtnl_close(&rth); + return rc; +} + unsigned ll_name_to_index(const char *name) { const struct ll_cache *im; @@ -204,7 +244,9 @@ unsigned ll_name_to_index(const char *name) if (im) return im->index; - idx = if_nametoindex(name); + idx = ll_link_get(name); + if (idx == 0) + idx = if_nametoindex(name); if (idx == 0) idx = ll_idx_a2n(name); return idx;