[PATCH iproute2-next 0/3] Improve batch times by caching link lookups

[PATCH iproute2-next 0/3] Improve batch times by caching link lookups

[David Ahern]

From: David Ahern <dsahern@gmail.com>

Many commands convert device names to an index using ll_name_to_index.
ll_name_to_index calls if_nametoindex which is ioctl based and the
result is not cached. When using a batch file this means the same device
lookups can be done repeatedly adding unnecessary overhead
(socket + ioctl + close for each device lookup).

This series adds a new function, ll_link_get, to send a netlink based
RTM_GETLINK. If successful, the result is cached in idx_head and name_head
so future lookups can re-use the entry. ll_name_to_index is updated to use
ll_link_get over if_nametoindex.

The first 2 patches add a means to drop an entry from the cache and updates
iplink_modify to use that new function to drop entries on device renames.

David Ahern (3):
  ll_map: Add function to remove link cache entry by index
  ip link: Drop cache entry on name changes
  Improve batch times by caching link lookups

 include/ll_map.h  |  1 +
 ip/ip_common.h    |  3 ++-
 ip/iplink.c       | 10 ++++++++--
 ip/iplink_vxcan.c |  3 ++-
 ip/link_veth.c    |  3 ++-
 lib/ll_map.c      | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 6 files changed, 72 insertions(+), 6 deletions(-)

-- 
2.11.0

[PATCH iproute2-next 1/3] ll_map: Add function to remove link cache entry by index

[David Ahern]

From: David Ahern <dsahern@gmail.com>

Add ll_drop_by_index to remove an entry from the link cache.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 include/ll_map.h |  1 +
 lib/ll_map.c     | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/ll_map.h b/include/ll_map.h
index 511fe00b8567..4de1041e2746 100644
--- a/include/ll_map.h
+++ b/include/ll_map.h
@@ -9,6 +9,7 @@ unsigned ll_name_to_index(const char *name);
 const char *ll_index_to_name(unsigned idx);
 int ll_index_to_type(unsigned idx);
 int ll_index_to_flags(unsigned idx);
+void ll_drop_by_index(unsigned index);
 unsigned namehash(const char *str);
 
 const char *ll_idx_n2a(unsigned int idx);
diff --git a/lib/ll_map.c b/lib/ll_map.c
index 1ab8ef0758ac..8e8a0b1e9c9d 100644
--- a/lib/ll_map.c
+++ b/lib/ll_map.c
@@ -210,6 +210,20 @@ unsigned ll_name_to_index(const char *name)
 	return idx;
 }
 
+void ll_drop_by_index(unsigned index)
+{
+	struct ll_cache *im;
+
+	im = ll_get_by_index(index);
+	if (!im)
+		return;
+
+	hlist_del(&im->idx_hash);
+	hlist_del(&im->name_hash);
+
+	free(im);
+}
+
 void ll_init_map(struct rtnl_handle *rth)
 {
 	static int initialized;
-- 
2.11.0

[PATCH iproute2-next 2/3] ip link: Drop cache entry on name changes

[David Ahern]

From: David Ahern <dsahern@gmail.com>

If a link's name is changed remove any entry from the link cache.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 ip/ip_common.h    |  3 ++-
 ip/iplink.c       | 10 ++++++++--
 ip/iplink_vxcan.c |  3 ++-
 ip/link_veth.c    |  3 ++-
 4 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/ip/ip_common.h b/ip/ip_common.h
index d67575c63c24..ae2b2fb4e314 100644
--- a/ip/ip_common.h
+++ b/ip/ip_common.h
@@ -123,7 +123,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 *name_change);
 
 /* 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 b5519201fef7..65838e2b3fe6 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -573,7 +573,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 *name_change)
 {
 	char *name = NULL;
 	char *dev = NULL;
@@ -943,6 +944,8 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
 		dev = name;
 	else if (!strcmp(name, dev))
 		name = dev;
+	else if (name_change)
+		*name_change = true;
 
 	if (dev && addr_len) {
 		int halen = nl_get_ll_addr_len(dev);
@@ -1030,9 +1033,10 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 		.n.nlmsg_type = cmd,
 		.i.ifi_family = preferred_family,
 	};
+	bool name_change = false;
 	int ret;
 
-	ret = iplink_parse(argc, argv, &req, &type);
+	ret = iplink_parse(argc, argv, &req, &type, &name_change);
 	if (ret < 0)
 		return ret;
 
@@ -1082,6 +1086,8 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 
 	if (rtnl_talk(&rth, &req.n, NULL) < 0)
 		return -2;
+	else if (name_change)
+		ll_drop_by_index(req.i.ifi_index);
 
 	return 0;
 }
diff --git a/ip/iplink_vxcan.c b/ip/iplink_vxcan.c
index 8b08c9a70c65..bdd372037836 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,
+			   NULL);
 	if (err < 0)
 		return err;
 
diff --git a/ip/link_veth.c b/ip/link_veth.c
index 33e8f2b102e7..ef0bc1948323 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,
+			   NULL);
 	if (err < 0)
 		return err;
 
-- 
2.11.0

Re: [PATCH iproute2-next 2/3] ip link: Drop cache entry on name changes

[Stephen Hemminger]

On Mon,  7 Jan 2019 14:55:51 -0800
David Ahern <dsahern@kernel.org> wrote:

> +int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type,
> +		 bool *name_change);

Not a real fan of adding another by reference return value flag.
It makes the logic flow more complex.

Is there another way? Caching in general is dicey anyway.

Re: [PATCH iproute2-next 2/3] ip link: Drop cache entry on name changes

[David Ahern]

On 1/7/19 5:06 PM, Stephen Hemminger wrote:
> On Mon,  7 Jan 2019 14:55:51 -0800
> David Ahern <dsahern@kernel.org> wrote:
> 
>> +int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type,
>> +		 bool *name_change);
> 
> Not a real fan of adding another by reference return value flag.
> It makes the logic flow more complex.
> 
> Is there another way? Caching in general is dicey anyway.
> 

The details of the link change are buried inside of the req argument. It
does not make sense to decode the message to see if the devices
referenced by ifi_index == IFLA_NAME.

The alternative is to just negate caching of ifi_index regardless of
what the request is doing.

If the caching only gained 5 or 10% we would not be having this
discussion - I would not have sent patches. The gain here is significant
and the caching aligns with the whole intent of batch files.

[PATCH iproute2-next 3/3] Improve batch times by caching link lookups

[David Ahern]

From: David Ahern <dsahern@gmail.com>

ip route (for example) 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 + close 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. Update ll_name_to_index to use ll_link_get over
if_nametoindex.

With this change the time to install 720,022 routes with 2 ecmp nexthops
where the nexthop device is given is reduced from 30.7 seconds to 17.6
seconds.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 lib/ll_map.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/lib/ll_map.c b/lib/ll_map.c
index 8e8a0b1e9c9d..46c78df08c11 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;
-- 
2.11.0

Re: [PATCH iproute2-next 3/3] Improve batch times by caching link lookups

[Stephen Hemminger]

On Mon,  7 Jan 2019 14:55:52 -0800
David Ahern <dsahern@kernel.org> wrote:

> +	idx = ll_link_get(name);
> +	if (idx == 0)
> +		idx = if_nametoindex(name);

What is advantage of using netlink, other than not having to open
a socket.

Re: [PATCH iproute2-next 3/3] Improve batch times by caching link lookups

[David Ahern]

On 1/7/19 5:05 PM, Stephen Hemminger wrote:
> On Mon,  7 Jan 2019 14:55:52 -0800
> David Ahern <dsahern@kernel.org> wrote:
> 
>> +	idx = ll_link_get(name);
>> +	if (idx == 0)
>> +		idx = if_nametoindex(name);
> 
> What is advantage of using netlink, other than not having to open
> a socket.
> 

As mentioned in my response to Eric, use of netlink is to keep the cache
entries in sync. The existing cache is populated by netlink messages;
this function should follow suit. if_nametoindex does not allow setting
the flags or type fields.