Skip to contentSkip to conversation

neighbor: Add protocol attribute

PATCH In review David Ahern 4 participants 9 messages Dec 7, 2018 – Dec 14, 2018

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

David Ahern

author
#0 lore raw

PATCH 1/1 neighbor: Add protocol attribute

From: David Ahern <[email protected]>

Similar to routes and rules, add protocol attribute to neighbor entries for easier tracking of how each was created.

Signed-off-by David Ahern <[email protected]>
---
 include/net/neighbour.h        |  2 ++
 include/uapi/linux/neighbour.h |  1 +
 net/core/neighbour.c           | 24 +++++++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
include/net/neighbour.h +2 -0
index 6c13072910ab..e93c59df9501 100644
@@ -149,6 +149,7 @@ struct neighbour {
 	__u8			nud_state;
 	__u8			type;
 	__u8			dead;
+	u8			protocol;
 	seqlock_t		ha_lock;
 	unsigned char		ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned long))];
 	struct hh_cache		hh;

@@ -173,6 +174,7 @@ struct pneigh_entry {
 	possible_net_t		net;
 	struct net_device	*dev;
 	u8			flags;
+	u8			protocol;
 	u8			key[0];
 };
include/uapi/linux/neighbour.h +1 -0
index 998155444e0d..cd144e3099a3 100644
@@ -28,6 +28,7 @@ enum {
 	NDA_MASTER,
 	NDA_LINK_NETNSID,
 	NDA_SRC_VNI,
+	NDA_PROTOCOL,  /* Originator of entry */
 	__NDA_MAX
 };
net/core/neighbour.c +23 -1
index c3b58712e98b..56984695585d 100644
@@ -1799,6 +1799,7 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct net_device *dev = NULL;
 	struct neighbour *neigh;
 	void *dst, *lladdr;
+	u8 protocol = 0;
 	int err;
 
 	ASSERT_RTNL();

@@ -1838,6 +1839,14 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	dst = nla_data(tb[NDA_DST]);
 	lladdr = tb[NDA_LLADDR] ? nla_data(tb[NDA_LLADDR]) : NULL;
 
+	if (tb[NDA_PROTOCOL]) {
+		if (nla_len(tb[NDA_PROTOCOL]) != sizeof(u8)) {
+			NL_SET_ERR_MSG(extack, "Invalid protocol attribute");
+			goto out;
+		}
+		protocol = nla_get_u8(tb[NDA_PROTOCOL]);
+	}
+
 	if (ndm->ndm_flags & NTF_PROXY) {
 		struct pneigh_entry *pn;
 

@@ -1845,6 +1854,8 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 		pn = pneigh_lookup(tbl, net, dst, dev, 1);
 		if (pn) {
 			pn->flags = ndm->ndm_flags;
+			if (protocol)
+				pn->protocol = protocol;
 			err = 0;
 		}
 		goto out;

@@ -1893,6 +1904,10 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh,
 	} else
 		err = __neigh_update(neigh, lladdr, ndm->ndm_state, flags,
 				     NETLINK_CB(skb).portid, extack);
+
+	if (protocol)
+		neigh->protocol = protocol;
+
 	neigh_release(neigh);
 
 out:

@@ -2386,6 +2401,9 @@ static int neigh_fill_info(struct sk_buff *skb, struct neighbour *neigh,
 	    nla_put(skb, NDA_CACHEINFO, sizeof(ci), &ci))
 		goto nla_put_failure;
 
+	if (neigh->protocol && nla_put_u8(skb, NDA_PROTOCOL, neigh->protocol))
+		goto nla_put_failure;
+
 	nlmsg_end(skb, nlh);
 	return 0;
 

@@ -2417,6 +2435,9 @@ static int pneigh_fill_info(struct sk_buff *skb, struct pneigh_entry *pn,
 	if (nla_put(skb, NDA_DST, tbl->key_len, pn->key))
 		goto nla_put_failure;
 
+	if (pn->protocol && nla_put_u8(skb, NDA_PROTOCOL, pn->protocol))
+		goto nla_put_failure;
+
 	nlmsg_end(skb, nlh);
 	return 0;
 

@@ -3072,7 +3093,8 @@ static inline size_t neigh_nlmsg_size(void)
 	       + nla_total_size(MAX_ADDR_LEN) /* NDA_DST */
 	       + nla_total_size(MAX_ADDR_LEN) /* NDA_LLADDR */
 	       + nla_total_size(sizeof(struct nda_cacheinfo))
-	       + nla_total_size(4); /* NDA_PROBES */
+	       + nla_total_size(4)  /* NDA_PROBES */
+	       + nla_total_size(1); /* NDA_PROTOCOL */
 }
 
 static void __neigh_notify(struct neighbour *n, int type, int flags,
Signature · 1 line
2.11.0
#1 lore raw
Quoted 17 lines · David Ahern

From: David Ahern <[email protected]>

Similar to routes and rules, add protocol attribute to neighbor entries for easier tracking of how each was created.

Signed-off-by David Ahern <[email protected]>
---
 include/net/neighbour.h        |  2 ++
 include/uapi/linux/neighbour.h |  1 +
 net/core/neighbour.c           | 24 +++++++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
include/net/neighbour.h +0 -0
index 6c13072910ab..e93c59df9501 100644

include/net/neighbour.h struct neighbour { · line 154

Quoted 6 lines
include/net/neighbour.h +1 -0
@@ -149,6 +149,7 @@ struct neighbour {
 	__u8			nud_state;
 	__u8			type;
 	__u8			dead;
+	u8			protocol;
 	seqlock_t		ha_lock;
 	unsigned char		ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned long))];

This looks like ha[] alignment would change, I am not sure how critical it is.

Quoted 9 lines
include/net/neighbour.h +1 -0
 	struct hh_cache		hh;

@@ -173,6 +174,7 @@ struct pneigh_entry {
 	possible_net_t		net;
 	struct net_device	*dev;
 	u8			flags;
+	u8			protocol;
 	u8			key[0];
 };
#2 lore raw
Quoted 15 lines · Eric Dumazet
Quoted 12 lines · David Ahern

From: David Ahern <[email protected]>

Similar to routes and rules, add protocol attribute to neighbor entries for easier tracking of how each was created.

Signed-off-by David Ahern <[email protected]>
---
 include/net/neighbour.h        |  2 ++
 include/uapi/linux/neighbour.h |  1 +
 net/core/neighbour.c           | 24 +++++++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
Quoted 13 lines

include/net/neighbour.h struct neighbour { · line 154

Quoted 11 lines
include/net/neighbour.h +1 -0
index 6c13072910ab..e93c59df9501 100644
@@ -149,6 +149,7 @@ struct neighbour {
 	__u8			nud_state;
 	__u8			type;
 	__u8			dead;
+	u8			protocol;
 	seqlock_t		ha_lock;
 	unsigned char		ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned long))];

This looks like ha[] alignment would change, I am not sure how critical it is.

Just adds 4 bytes to neighbour:

...
/* --- cacheline 2 boundary (128 bytes) --- */

long unsigned int          used;                 /*   128     8 */
atomic_t                   probes;               /*   136     4 */
__u8                       flags;                /*   140     1 */
__u8                       nud_state;            /*   141     1 */
__u8                       type;                 /*   142     1 */
__u8                       dead;                 /*   143     1 */
u8                         protocol;             /*   144     1 */

/* XXX 3 bytes hole, try to pack */

seqlock_t                  ha_lock;              /*   148     8 */
unsigned char              ha[32];               /*   156    32 */

/* XXX 4 bytes hole, try to pack */

/* --- cacheline 3 boundary (192 bytes) --- */
struct hh_cache hh; /* 192 48 */

...

but does not change the actual allocation size which is rounded to 512.

#3 lore raw
Quoted 45 lines · David Ahern
Quoted 15 lines · Eric Dumazet
Quoted 12 lines · David Ahern

From: David Ahern <[email protected]>

Similar to routes and rules, add protocol attribute to neighbor entries for easier tracking of how each was created.

Signed-off-by David Ahern <[email protected]>
---
 include/net/neighbour.h        |  2 ++
 include/uapi/linux/neighbour.h |  1 +
 net/core/neighbour.c           | 24 +++++++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
Quoted 13 lines

include/net/neighbour.h struct neighbour { · line 154

Quoted 11 lines
include/net/neighbour.h +1 -0
index 6c13072910ab..e93c59df9501 100644
@@ -149,6 +149,7 @@ struct neighbour {
 	__u8			nud_state;
 	__u8			type;
 	__u8			dead;
+	u8			protocol;
 	seqlock_t		ha_lock;
 	unsigned char		ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned long))];

This looks like ha[] alignment would change, I am not sure how critical it is.

Just adds 4 bytes to neighbour:

...
/* --- cacheline 2 boundary (128 bytes) --- */

long unsigned int          used;                 /*   128     8 */
atomic_t                   probes;               /*   136     4 */
__u8                       flags;                /*   140     1 */
__u8                       nud_state;            /*   141     1 */
__u8                       type;                 /*   142     1 */
__u8                       dead;                 /*   143     1 */
u8                         protocol;             /*   144     1 */

/* XXX 3 bytes hole, try to pack */

seqlock_t                  ha_lock;              /*   148     8 */
unsigned char              ha[32];               /*   156    32 */
Quoted 9 lines

/* XXX 4 bytes hole, try to pack */

/* --- cacheline 3 boundary (192 bytes) --- */
struct hh_cache hh; /* 192 48 */

...

but does not change the actual allocation size which is rounded to 512.

I have not talked about the allocation size, but alignment of ->ha field, which is kind of assuming long alignment, in a strange way.

As I said, I do not know how performance critical this might be.

maintainer
#4 lore raw

From: Eric Dumazet <[email protected]>
Date: Fri, 7 Dec 2018 15:03:04 -0800

Quoted 6 lines · David Ahern
Quoted 5 lines · Eric Dumazet

/* --- cacheline 3 boundary (192 bytes) --- */
struct hh_cache hh; /* 192 48 */

Quoted 7 lines
Quoted 4 lines

...

but does not change the actual allocation size which is rounded to 512.

I have not talked about the allocation size, but alignment of ->ha field, which is kind of assuming long alignment, in a strange way.

Right, neigh->ha[] should probably be kept 8-byte aligned.

#5 lore raw
Quoted 5 lines · David Miller

Right, neigh->ha[] should probably be kept 8-byte aligned.

From what I can see ha is only used with memcpy, and neighbour struct is

annotated with __randomize_layout. Are you saying that ha should be marked with __aligned(8)?

@@ -150,7 +150,7 @@ struct neighbour {
        __u8                    type;
        __u8                    dead;
        seqlock_t               ha_lock;

- unsigned char ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned
long))];
+ unsigned char ha[ALIGN(MAX_ADDR_LEN, sizeof(unsigned
long))] __aligned(8);
struct hh_cache hh;
int (*output)(struct neighbour *, struct

sk_buff *);
        const struct neigh_ops  *ops;
maintainer
#6 lore raw

From: David Ahern <[email protected]>
Date: Mon, 10 Dec 2018 19:47:33 -0700

Quoted 8 lines · David Miller
Quoted 3 lines

Right, neigh->ha[] should probably be kept 8-byte aligned.

From what I can see ha is only used with memcpy, and neighbour struct is annotated with __randomize_layout. Are you saying that ha should be marked with __aligned(8)?

People who care about performance probably don't build with randomization enabled, do they?

Even though it uses memcpy() it will be faster if it is 8 byte aligned and we can probably explicitly take advantage of that alignment even more if we add the marking as you suggest perhaps.

Given all of this, what is your opinion?

#7 lore raw
Quoted 12 lines · David Miller

From: David Ahern <[email protected]>
Date: Mon, 10 Dec 2018 19:47:33 -0700

Quoted 8 lines · David Miller
Quoted 3 lines

Right, neigh->ha[] should probably be kept 8-byte aligned.

From what I can see ha is only used with memcpy, and neighbour struct is annotated with __randomize_layout. Are you saying that ha should be marked with __aligned(8)?

Quoted 9 lines

People who care about performance probably don't build with randomization enabled, do they?

Even though it uses memcpy() it will be faster if it is 8 byte aligned and we can probably explicitly take advantage of that alignment even more if we add the marking as you suggest perhaps.

Given all of this, what is your opinion?

Arguably my take is ethernet centric. I do not see how 8-byte alignment matters when copying 6 bytes. In my response to Eric I showed ha is still 4-byte aligned and does not straddle cachelines. Those seem the more relevant to me.

maintainer
#8 lore raw

From: David Ahern <[email protected]>
Date: Tue, 11 Dec 2018 08:22:04 -0700

Quoted 20 lines · David Miller
Quoted 19 lines

From: David Ahern <[email protected]>
Date: Mon, 10 Dec 2018 19:47:33 -0700

Quoted 8 lines · David Miller
Quoted 3 lines

Right, neigh->ha[] should probably be kept 8-byte aligned.

From what I can see ha is only used with memcpy, and neighbour struct is annotated with __randomize_layout. Are you saying that ha should be marked with __aligned(8)?

People who care about performance probably don't build with randomization enabled, do they?

Even though it uses memcpy() it will be faster if it is 8 byte aligned and we can probably explicitly take advantage of that alignment even more if we add the marking as you suggest perhaps.

Quoted 7 lines
Quoted 2 lines

Given all of this, what is your opinion?

Arguably my take is ethernet centric. I do not see how 8-byte alignment matters when copying 6 bytes. In my response to Eric I showed ha is still 4-byte aligned and does not straddle cachelines. Those seem the more relevant to me.

Ok, please resubmit as-is if you like.