neighbor: cleanups plus extack for add and delete

PATCH In review David Ahern 3 participants 12 messages Dec 5, 2018 – Dec 6, 2018

Subsystems (5)

Networking Network drivers BPF IPv4/IPv6 +1 more subsystems

lore

Share & tools

lore

David Ahern

author maintainer

2018-12-05 23:33 UTC #0 lore raw

cover neighbor: cleanups plus extack for add and delete

From: David Ahern <[email protected]>

cleanups:
- remove open coding of key and hash functions for ipv4 and ipv6 and then collapse hash functions
- collapse now unnecessary ___neigh_lookup_noref helper
- create helper for neigh hash computation
- remove duplicate lookup in neigh_add

After that add extack messages for neighbor add and delete.

David Ahern (7):
neighbor: Remove open coding of key and hash functions
neighbor: Fold ___neigh_lookup_noref into __neigh_lookup_noref net/ipv4: Move arp_hashfn into arp_hash
net/ipv6: Move ndisc_hashfn to ndisc_hash
neighbor: Create a neigh_hash helper
neighbor: Skip the duplicate lookup in neigh_add
neighbor: Add extack messages for add and delete commands

 include/net/arp.h       | 10 +------
 include/net/ndisc.h     | 12 +--------
 include/net/neighbour.h | 30 +++++++++------------
 net/core/filter.c       |  3 +--
 net/core/neighbour.c    | 72 ++++++++++++++++++++++++++++++-------------------
 net/ipv4/arp.c          |  5 +++-
 net/ipv6/ndisc.c        |  7 ++++-
 7 files changed, 71 insertions(+), 68 deletions(-)

Signature · 1 line

2.11.0

David Ahern

author maintainer

2018-12-05 23:33 UTC #1 lore raw

PATCH 1/7 neighbor: Remove open coding of key and hash functions

From: David Ahern <[email protected]>

___neigh_lookup_noref takes the key and hash functions as inputs, yet those are part of the operations listed in the neigh_table which is also passed as an arugment. Remove the open coding of these internal implementations by converting uses of ___neigh_lookup_noref to __neigh_lookup_noref.

For IPv4, arp_key_eq is essentially a call to neigh_key_eq32 and arp_hash is a call to arp_hashfn. Similarly for IPv6, ndisc_key_eq calls neigh_key_eq128 and ndisc_hash calls ndisc_hashfn. So the change in helpers is a no-op.

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

---
 include/net/arp.h   | 2 +-
 include/net/ndisc.h | 2 +-
 net/core/filter.c   | 3 +--
 3 files changed, 3 insertions(+), 4 deletions(-)

include/net/arp.h +1 -1

index 977aabfcdc03..a5091f13cd3e 100644
@@ -23,7 +23,7 @@ static inline struct neighbour *__ipv4_neigh_lookup_noref(struct net_device *dev
 	if (dev->flags & (IFF_LOOPBACK | IFF_POINTOPOINT))
 		key = INADDR_ANY;
 
-	return ___neigh_lookup_noref(&arp_tbl, neigh_key_eq32, arp_hashfn, &key, dev);
+	return __neigh_lookup_noref(&arp_tbl, &key, dev);
 }
 
 static inline struct neighbour *__ipv4_neigh_lookup(struct net_device *dev, u32 key)

include/net/ndisc.h +1 -1

index ddfbb591e2c5..c354345c679b 100644
@@ -376,7 +376,7 @@ static inline u32 ndisc_hashfn(const void *pkey, const struct net_device *dev, _
 
 static inline struct neighbour *__ipv6_neigh_lookup_noref(struct net_device *dev, const void *pkey)
 {
-	return ___neigh_lookup_noref(&nd_tbl, neigh_key_eq128, ndisc_hashfn, pkey, dev);
+	return __neigh_lookup_noref(&nd_tbl, pkey, dev);
 }
 
 static inline struct neighbour *__ipv6_neigh_lookup(struct net_device *dev, const void *pkey)

net/core/filter.c +1 -2

index bd0df75dc7b6..f10cc675783c 100644
@@ -4668,8 +4668,7 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
 	 * not needed here. Can not use __ipv6_neigh_lookup_noref here
 	 * because we need to get nd_tbl via the stub
 	 */
-	neigh = ___neigh_lookup_noref(ipv6_stub->nd_tbl, neigh_key_eq128,
-				      ndisc_hashfn, dst, dev);
+	neigh = __neigh_lookup_noref(ipv6_stub->nd_tbl, dst, dev);
 	if (!neigh)
 		return BPF_FIB_LKUP_RET_NO_NEIGH;

Signature · 1 line

2.11.0

David Ahern

author maintainer

2018-12-05 23:33 UTC #2 lore raw

PATCH 2/7 neighbor: Fold ___neigh_lookup_noref into __neigh_lookup_noref

In reply to David Ahern

From: David Ahern <[email protected]>

There are no more direct callers of ___neigh_lookup_noref so no need for it to be a standalone helper.

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

---
 include/net/neighbour.h | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

include/net/neighbour.h +5 ~~-17~~

index f58b384aa6c9..aac87bc2d96b 100644
@@ -270,37 +270,25 @@ static inline bool neigh_key_eq128(const struct neighbour *n, const void *pkey)
 		(n32[2] ^ p32[2]) | (n32[3] ^ p32[3])) == 0;
 }
 
-static inline struct neighbour *___neigh_lookup_noref(
-	struct neigh_table *tbl,
-	bool (*key_eq)(const struct neighbour *n, const void *pkey),
-	__u32 (*hash)(const void *pkey,
-		      const struct net_device *dev,
-		      __u32 *hash_rnd),
-	const void *pkey,
-	struct net_device *dev)
+static inline struct neighbour *__neigh_lookup_noref(struct neigh_table *tbl,
+						     const void *pkey,
+						     struct net_device *dev)
 {
 	struct neigh_hash_table *nht = rcu_dereference_bh(tbl->nht);
 	struct neighbour *n;
 	u32 hash_val;
 
-	hash_val = hash(pkey, dev, nht->hash_rnd) >> (32 - nht->hash_shift);
+	hash_val = tbl->hash(pkey, dev, nht->hash_rnd) >> (32 - nht->hash_shift);
 	for (n = rcu_dereference_bh(nht->hash_buckets[hash_val]);
 	     n != NULL;
 	     n = rcu_dereference_bh(n->next)) {
-		if (n->dev == dev && key_eq(n, pkey))
+		if (n->dev == dev && tbl->key_eq(n, pkey))
 			return n;
 	}
 
 	return NULL;
 }
 
-static inline struct neighbour *__neigh_lookup_noref(struct neigh_table *tbl,
-						     const void *pkey,
-						     struct net_device *dev)
-{
-	return ___neigh_lookup_noref(tbl, tbl->key_eq, tbl->hash, pkey, dev);
-}
-
 void neigh_table_init(int index, struct neigh_table *tbl);
 int neigh_table_clear(int index, struct neigh_table *tbl);
 struct neighbour *neigh_lookup(struct neigh_table *tbl, const void *pkey,

Signature · 1 line

2.11.0

David Ahern

author maintainer

2018-12-05 23:33 UTC #7 lore raw

PATCH 3/7 net/ipv4: Move arp_hashfn into arp_hash

In reply to David Ahern

From: David Ahern <[email protected]>

There are no more direct references to arp_hashfn so fold it into arp_hash, the hash callback for arp.

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

---
 include/net/arp.h | 8 --------
 net/ipv4/arp.c    | 5 ++++-
 2 files changed, 4 insertions(+), 9 deletions(-)

include/net/arp.h +0 -8

index a5091f13cd3e..9f433c077b67 100644
@@ -10,14 +10,6 @@
 
 extern struct neigh_table arp_tbl;
 
-static inline u32 arp_hashfn(const void *pkey, const struct net_device *dev, u32 *hash_rnd)
-{
-	u32 key = *(const u32 *)pkey;
-	u32 val = key ^ hash32_ptr(dev);
-
-	return val * hash_rnd[0];
-}
-
 static inline struct neighbour *__ipv4_neigh_lookup_noref(struct net_device *dev, u32 key)
 {
 	if (dev->flags & (IFF_LOOPBACK | IFF_POINTOPOINT))

net/ipv4/arp.c +4 -1

index 850a6f13a082..6b88211287ae 100644
@@ -213,7 +213,10 @@ static u32 arp_hash(const void *pkey,
 		    const struct net_device *dev,
 		    __u32 *hash_rnd)
 {
-	return arp_hashfn(pkey, dev, hash_rnd);
+	u32 key = *(const u32 *)pkey;
+	u32 val = key ^ hash32_ptr(dev);
+
+	return val * hash_rnd[0];
 }
 
 static bool arp_key_eq(const struct neighbour *neigh, const void *pkey)

Signature · 1 line

2.11.0

David Ahern

author maintainer

2018-12-05 23:33 UTC #8 lore raw

PATCH 4/7 net/ipv6: Move ndisc_hashfn to ndisc_hash

In reply to David Ahern

From: David Ahern <[email protected]>

There are no more direct references to ndisc_hashfn so fold it into ndisc_hash, the hash callback for ndisc.

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

---
 include/net/ndisc.h | 10 ----------
 net/ipv6/ndisc.c    |  7 ++++++-
 2 files changed, 6 insertions(+), 11 deletions(-)

include/net/ndisc.h +0 ~~-10~~

index c354345c679b..83a84f68901b 100644
@@ -364,16 +364,6 @@ static inline u8 *ndisc_opt_addr_data(struct nd_opt_hdr *p,
 				     ndisc_addr_option_pad(dev->type));
 }
 
-static inline u32 ndisc_hashfn(const void *pkey, const struct net_device *dev, __u32 *hash_rnd)
-{
-	const u32 *p32 = pkey;
-
-	return (((p32[0] ^ hash32_ptr(dev)) * hash_rnd[0]) +
-		(p32[1] * hash_rnd[1]) +
-		(p32[2] * hash_rnd[2]) +
-		(p32[3] * hash_rnd[3]));
-}
-
 static inline struct neighbour *__ipv6_neigh_lookup_noref(struct net_device *dev, const void *pkey)
 {
 	return __neigh_lookup_noref(&nd_tbl, pkey, dev);

net/ipv6/ndisc.c +6 -1

index 659ecf4e4b3c..304a32b3c3f5 100644
@@ -311,7 +311,12 @@ static u32 ndisc_hash(const void *pkey,
 		      const struct net_device *dev,
 		      __u32 *hash_rnd)
 {
-	return ndisc_hashfn(pkey, dev, hash_rnd);
+	const u32 *p32 = pkey;
+
+	return (((p32[0] ^ hash32_ptr(dev)) * hash_rnd[0]) +
+		 (p32[1] * hash_rnd[1]) +
+		 (p32[2] * hash_rnd[2]) +
+		 (p32[3] * hash_rnd[3]));
 }
 
 static bool ndisc_key_eq(const struct neighbour *n, const void *pkey)

Signature · 1 line

2.11.0

David Ahern

author maintainer

2018-12-05 23:33 UTC #9 lore raw

PATCH 5/7 neighbor: Create a neigh_hash helper

In reply to David Ahern

From: David Ahern <[email protected]>

Consolidate calculations of the neighbor hash into a single helper.

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

---
 include/net/neighbour.h | 10 +++++++++-
 net/core/neighbour.c    | 15 +++++----------
 2 files changed, 14 insertions(+), 11 deletions(-)

include/net/neighbour.h +9 -1

index aac87bc2d96b..092493a8c91b 100644
@@ -270,6 +270,14 @@ static inline bool neigh_key_eq128(const struct neighbour *n, const void *pkey)
 		(n32[2] ^ p32[2]) | (n32[3] ^ p32[3])) == 0;
 }
 
+static inline u32 neigh_hash(struct neigh_table *tbl,
+			     struct neigh_hash_table *nht,
+			     const void *pkey,
+			     struct net_device *dev)
+{
+	return tbl->hash(pkey, dev, nht->hash_rnd) >> (32 - nht->hash_shift);
+}
+
 static inline struct neighbour *__neigh_lookup_noref(struct neigh_table *tbl,
 						     const void *pkey,
 						     struct net_device *dev)

@@ -278,7 +286,7 @@ static inline struct neighbour *__neigh_lookup_noref(struct neigh_table *tbl,
 	struct neighbour *n;
 	u32 hash_val;
 
-	hash_val = tbl->hash(pkey, dev, nht->hash_rnd) >> (32 - nht->hash_shift);
+	hash_val = neigh_hash(tbl, nht, pkey, dev);
 	for (n = rcu_dereference_bh(nht->hash_buckets[hash_val]);
 	     n != NULL;
 	     n = rcu_dereference_bh(n->next)) {

net/core/neighbour.c +5 ~~-10~~

index 41954e42a2de..53e30c15882d 100644
@@ -151,9 +151,8 @@ bool neigh_remove_one(struct neighbour *ndel, struct neigh_table *tbl)
 
 	nht = rcu_dereference_protected(tbl->nht,
 					lockdep_is_held(&tbl->lock));
-	hash_val = tbl->hash(pkey, ndel->dev, nht->hash_rnd);
-	hash_val = hash_val >> (32 - nht->hash_shift);
 
+	hash_val = neigh_hash(tbl, nht, pkey, ndel->dev);
 	np = &nht->hash_buckets[hash_val];
 	while ((n = rcu_dereference_protected(*np,
 					      lockdep_is_held(&tbl->lock)))) {

@@ -434,10 +433,7 @@ static struct neigh_hash_table *neigh_hash_grow(struct neigh_table *tbl,
 						   lockdep_is_held(&tbl->lock));
 		     n != NULL;
 		     n = next) {
-			hash = tbl->hash(n->primary_key, n->dev,
-					 new_nht->hash_rnd);
-
-			hash >>= (32 - new_nht->hash_shift);
+			hash = neigh_hash(tbl, new_nht, n->primary_key, n->dev);
 			next = rcu_dereference_protected(n->next,
 						lockdep_is_held(&tbl->lock));

@@ -485,9 +481,9 @@ struct neighbour *neigh_lookup_nodev(struct neigh_table *tbl, struct net *net,
 	NEIGH_CACHE_STAT_INC(tbl, lookups);
 
 	rcu_read_lock_bh();
-	nht = rcu_dereference_bh(tbl->nht);
-	hash_val = tbl->hash(pkey, NULL, nht->hash_rnd) >> (32 - nht->hash_shift);
 
+	nht = rcu_dereference_bh(tbl->nht);
+	hash_val = neigh_hash(tbl, nht, pkey, NULL);
 	for (n = rcu_dereference_bh(nht->hash_buckets[hash_val]);
 	     n != NULL;
 	     n = rcu_dereference_bh(n->next)) {

@@ -553,13 +549,12 @@ struct neighbour *__neigh_create(struct neigh_table *tbl, const void *pkey,
 	if (atomic_read(&tbl->entries) > (1 << nht->hash_shift))
 		nht = neigh_hash_grow(tbl, nht->hash_shift + 1);
 
-	hash_val = tbl->hash(n->primary_key, dev, nht->hash_rnd) >> (32 - nht->hash_shift);
-
 	if (n->parms->dead) {
 		rc = ERR_PTR(-EINVAL);
 		goto out_tbl_unlock;
 	}
 
+	hash_val = neigh_hash(tbl, nht, n->primary_key, dev);
 	for (n1 = rcu_dereference_protected(nht->hash_buckets[hash_val],
 					    lockdep_is_held(&tbl->lock));
 	     n1 != NULL;

Signature · 1 line

2.11.0

David Ahern

author maintainer

2018-12-05 23:33 UTC #10 lore raw

PATCH 6/7 neighbor: Skip the duplicate lookup in neigh_add

In reply to David Ahern

From: David Ahern <[email protected]>

When adding a new neighbor via rtnetlink, neigh_add does a lookup and if the result is NULL calls __neigh_lookup_errno to create a new entry if the NLM_F_CREATE flag is set. But, __neigh_lookup_errno calls neigh_lookup again before neigh_create; the neigh_lookup is redundant.

Replace the call to __neigh_lookup_errno with a call to __neigh_create to more efficiently achieve the same result and prepare for the next patch.

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

---
 net/core/neighbour.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/core/neighbour.c +1 -1

index 53e30c15882d..e324467e9a71 100644
@@ -1785,7 +1785,7 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 			goto out;
 		}
 
-		neigh = __neigh_lookup_errno(tbl, dst, dev);
+		neigh = __neigh_create(tbl, dst, dev, true);
 		if (IS_ERR(neigh)) {
 			err = PTR_ERR(neigh);
 			goto out;

Signature · 1 line

2.11.0

David Ahern

author maintainer

2018-12-05 23:33 UTC #11 lore raw

PATCH 7/7 neighbor: Add extack messages for add and delete commands

In reply to David Ahern

From: David Ahern <[email protected]>

Add extack messages for failures in neigh_add and neigh_delete.

Also, require NDA_DST length to be exactly the key length for the table otherwise it is an unexpected address and can lead to unexpected entries. e.g., IPv4 table sent and IPv6 address (using a modified ip):
$ ip neigh add 2001:db8:1::1 dev foo
$ ip neigh ls dev foo
32.1.13.184 dev foo lladdr 72:ed:f1:d9:20:9a PERMANENT

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

---
 net/core/neighbour.c | 55 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 16 deletions(-)

net/core/neighbour.c +39 ~~-16~~

index e324467e9a71..916a99fbb306 100644
@@ -1132,8 +1132,9 @@ static void neigh_update_hhs(struct neighbour *neigh)
    Caller MUST hold reference count on the entry.
  */
 
-int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
-		 u32 flags, u32 nlmsg_pid)
+static int __neigh_update(struct neighbour *neigh, const u8 *lladdr,
+			  u8 new, u32 flags, u32 nlmsg_pid,
+			  struct netlink_ext_ack *extack)
 {
 	u8 old;
 	int err;

@@ -1150,8 +1151,10 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
 	if (!(flags & NEIGH_UPDATE_F_ADMIN) &&
 	    (old & (NUD_NOARP | NUD_PERMANENT)))
 		goto out;
-	if (neigh->dead)
+	if (neigh->dead) {
+		NL_SET_ERR_MSG(extack, "Neighbor entry is now dead");
 		goto out;
+	}
 
 	neigh_update_ext_learned(neigh, flags, &notify);

@@ -1188,8 +1191,10 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
 		   use it, otherwise discard the request.
 		 */
 		err = -EINVAL;
-		if (!(old & NUD_VALID))
+		if (!(old & NUD_VALID)) {
+			NL_SET_ERR_MSG(extack, "No link layer address given");
 			goto out;
+		}
 		lladdr = neigh->ha;
 	}

@@ -1302,6 +1307,12 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
 
 	return err;
 }
+
+int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
+		 u32 flags, u32 nlmsg_pid)
+{
+	return __neigh_update(neigh, lladdr, new, flags, nlmsg_pid, NULL);
+}
 EXPORT_SYMBOL(neigh_update);
 
 /* Update the neigh to listen temporarily for probe responses, even if it is

@@ -1673,8 +1684,10 @@ static int neigh_delete(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto out;
 
 	dst_attr = nlmsg_find_attr(nlh, sizeof(*ndm), NDA_DST);
-	if (dst_attr == NULL)
+	if (!dst_attr) {
+		NL_SET_ERR_MSG(extack, "Network address not specified");
 		goto out;
+	}
 
 	ndm = nlmsg_data(nlh);
 	if (ndm->ndm_ifindex) {

@@ -1689,8 +1702,10 @@ static int neigh_delete(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (tbl == NULL)
 		return -EAFNOSUPPORT;
 
-	if (nla_len(dst_attr) < (int)tbl->key_len)
+	if (nla_len(dst_attr) < (int)tbl->key_len) {
+		NL_SET_ERR_MSG(extack, "Invalid network address");
 		goto out;
+	}
 
 	if (ndm->ndm_flags & NTF_PROXY) {
 		err = pneigh_delete(tbl, net, nla_data(dst_attr), dev);

@@ -1706,10 +1721,9 @@ static int neigh_delete(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto out;
 	}
 
-	err = neigh_update(neigh, NULL, NUD_FAILED,
-			   NEIGH_UPDATE_F_OVERRIDE |
-			   NEIGH_UPDATE_F_ADMIN,
-			   NETLINK_CB(skb).portid);
+	err = __neigh_update(neigh, NULL, NUD_FAILED,
+			     NEIGH_UPDATE_F_OVERRIDE | NEIGH_UPDATE_F_ADMIN,
+			     NETLINK_CB(skb).portid, extack);
 	write_lock_bh(&tbl->lock);
 	neigh_release(neigh);
 	neigh_remove_one(neigh, tbl);

@@ -1739,8 +1753,10 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto out;
 
 	err = -EINVAL;
-	if (tb[NDA_DST] == NULL)
+	if (!tb[NDA_DST]) {
+		NL_SET_ERR_MSG(extack, "Network address not specified");
 		goto out;
+	}
 
 	ndm = nlmsg_data(nlh);
 	if (ndm->ndm_ifindex) {

@@ -1750,16 +1766,21 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 			goto out;
 		}
 
-		if (tb[NDA_LLADDR] && nla_len(tb[NDA_LLADDR]) < dev->addr_len)
+		if (tb[NDA_LLADDR] && nla_len(tb[NDA_LLADDR]) < dev->addr_len) {
+			NL_SET_ERR_MSG(extack, "Invalid link address");
 			goto out;
+		}
 	}
 
 	tbl = neigh_find_table(ndm->ndm_family);
 	if (tbl == NULL)
 		return -EAFNOSUPPORT;
 
-	if (nla_len(tb[NDA_DST]) < (int)tbl->key_len)
+	if (nla_len(tb[NDA_DST]) != (int)tbl->key_len) {
+		NL_SET_ERR_MSG(extack, "Invalid network address");
 		goto out;
+	}
+
 	dst = nla_data(tb[NDA_DST]);
 	lladdr = tb[NDA_LLADDR] ? nla_data(tb[NDA_LLADDR]) : NULL;

@@ -1775,8 +1796,10 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto out;
 	}
 
-	if (dev == NULL)
+	if (!dev) {
+		NL_SET_ERR_MSG(extack, "Device not specified");
 		goto out;
+	}
 
 	neigh = neigh_lookup(tbl, dst, dev);
 	if (neigh == NULL) {

@@ -1812,8 +1835,8 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 		neigh_event_send(neigh, NULL);
 		err = 0;
 	} else
-		err = neigh_update(neigh, lladdr, ndm->ndm_state, flags,
-				   NETLINK_CB(skb).portid);
+		err = __neigh_update(neigh, lladdr, ndm->ndm_state, flags,
+				     NETLINK_CB(skb).portid, extack);
 	neigh_release(neigh);
 
 out:

Signature · 1 line

2.11.0

David Miller

maintainer

2018-12-06 00:44 UTC #3 lore raw

Re: [PATCH net-next 2/7] neighbor: Fold ___neigh_lookup_noref into __neigh_lookup_noref

In reply to David Ahern

From: David Ahern <[email protected]>
Date: Wed, 5 Dec 2018 15:34:09 -0800

Quoted 6 lines

include/net/neighbour.h +0 -2

@@ -270,37 +270,25 @@ static inline bool neigh_key_eq128(const struct neighbour *n, const void *pkey)
 		(n32[2] ^ p32[2]) | (n32[3] ^ p32[3])) == 0;
 }
 
-static inline struct neighbour *___neigh_lookup_noref(
-	struct neigh_table *tbl,

include/net/neighbour.h static inline bool neigh_key_eq128(const struct neighbour *n, const void *pkey) · line 276

Quoted 10 lines

include/net/neighbour.h +3 -6

-	bool (*key_eq)(const struct neighbour *n, const void *pkey),
-	__u32 (*hash)(const void *pkey,
-		      const struct net_device *dev,
-		      __u32 *hash_rnd),
-	const void *pkey,
-	struct net_device *dev)
+static inline struct neighbour *__neigh_lookup_noref(struct neigh_table *tbl,
+						     const void *pkey,
+						     struct net_device *dev)
 {

Sorry, we can't do this.

The whole point of how this is laid out is so that the entire hash traversal, including the hash function, is expanded inline.

This demux is extremely critical on the output side, it must be the smallest number of cycles possible. It was the only way I could justify not caching neigh entries in the routes any more when I wrote this code.

Even before retpoline, putting an indirect call here is painful. With retpoline it is deadly.

Please avoid removing the full inline expansion of the neigh lookup in the ipv6 and ipv4 data paths.

Thank you.

David Ahern

2018-12-06 00:46 UTC #4 lore raw

Re: [PATCH net-next 2/7] neighbor: Fold ___neigh_lookup_noref into __neigh_lookup_noref

Quoted 29 lines · David Miller

From: David Ahern <[email protected]>
Date: Wed, 5 Dec 2018 15:34:09 -0800
Quoted 6 lines
include/net/neighbour.h +0 -2
@@ -270,37 +270,25 @@ static inline bool neigh_key_eq128(const struct neighbour *n, const void *pkey)
 		(n32[2] ^ p32[2]) | (n32[3] ^ p32[3])) == 0;
 }
 
-static inline struct neighbour *___neigh_lookup_noref(
-	struct neigh_table *tbl,
include/net/neighbour.h static inline bool neigh_key_eq128(const struct neighbour *n, const void *pkey) · line 276
Quoted 10 lines
include/net/neighbour.h +3 -6
-	bool (*key_eq)(const struct neighbour *n, const void *pkey),
-	__u32 (*hash)(const void *pkey,
-		      const struct net_device *dev,
-		      __u32 *hash_rnd),
-	const void *pkey,
-	struct net_device *dev)
+static inline struct neighbour *__neigh_lookup_noref(struct neigh_table *tbl,
+						     const void *pkey,
+						     struct net_device *dev)
 {
Sorry, we can't do this.
The whole point of how this is laid out is so that the entire hash traversal, including the hash function, is expanded inline.
This demux is extremely critical on the output side, it must be the smallest number of cycles possible. It was the only way I could justify not caching neigh entries in the routes any more when I wrote this code.

Quoted 6 lines

Even before retpoline, putting an indirect call here is painful. With retpoline it is deadly.
Please avoid removing the full inline expansion of the neigh lookup in the ipv6 and ipv4 data paths.

ok. patches 5-7 are not dependent on 1-4. Should I re-send outside of this set?

David Ahern

2018-12-06 00:48 UTC #5 lore raw

Re: [PATCH net-next 2/7] neighbor: Fold ___neigh_lookup_noref into __neigh_lookup_noref

Quoted 2 lines · David Ahern

ok. patches 5-7 are not dependent on 1-4. Should I re-send outside of this set?

bleh. 5 is. I'll re-send.

David Miller

maintainer

2018-12-06 00:50 UTC #6 lore raw

Re: [PATCH net-next 2/7] neighbor: Fold ___neigh_lookup_noref into __neigh_lookup_noref

In reply to David Ahern

From: David Ahern <[email protected]>
Date: Wed, 5 Dec 2018 17:46:37 -0700

Quoted 2 lines

ok. patches 5-7 are not dependent on 1-4. Should I re-send outside of this set?

Yes, please respin.

Thanks David.