introduce bpf fdb lookup helper for xdp

net/bridge/br_fdb.c +21 -0

+
+	dev = dev_get_by_index_rcu(dev_net(ctx->rxq->dev), opt->ifindex);
+	if (!dev)
+		goto out;
+
+	if (unlikely(!netif_is_bridge_port(dev)))
+		goto out;
+
+	port = br_port_get_check_rcu(dev);
+	if (unlikely(!port || !port->br))
+		goto out;
+
+	dev = __br_fdb_find_port(port->br->dev, opt->addr, opt->vid, true);
+	if (dev)
+		ret = dev->ifindex;
+out:
+	rcu_read_unlock();
+
+	return ret;
+}
+
 struct net_bridge_fdb_entry *br_fdb_find_rcu(struct net_bridge *br,
 					     const unsigned char *addr,
 					     __u16 vid)

net/bridge/br_private.h +1 -0

index 2661dda1a92b..64d4f1727da2 100644
@@ -18,6 +18,7 @@
 #include <linux/if_vlan.h>
 #include <linux/rhashtable.h>
 #include <linux/refcount.h>
+#include <linux/bpf.h>
 
 #define BR_HASH_BITS 8
 #define BR_HASH_SIZE (1 << BR_HASH_BITS)

[ snip to focus on the API ]

net/bridge/br_fdb.c static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br, · line 287

Quoted 12 lines

net/bridge/br_fdb.c +12 -0

+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+				  struct bpf_fdb_lookup *opt,
+				  u32 opt__sz)
+{
+	struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
+	struct net_bridge_port *port;
+	struct net_device *dev;
+	int ret = -ENODEV;
+
+	BUILD_BUG_ON(sizeof(struct bpf_fdb_lookup) != NF_BPF_FDB_OPTS_SZ);
+	if (!opt || opt__sz != sizeof(struct bpf_fdb_lookup))
+		return -ENODEV;

Why is the BUILD_BUG_ON needed? Or why is the NF_BPF_FDB_OPTS_SZ constant even needed?

Review

Quoted 2 lines

(quoted diff) +1 -0

+	rcu_read_lock();

This is not needed when the function is only being called from XDP...

Quoted 45 lines

net/bridge/br_fdb.c +21 -0

+
+	dev = dev_get_by_index_rcu(dev_net(ctx->rxq->dev), opt->ifindex);
+	if (!dev)
+		goto out;
+
+	if (unlikely(!netif_is_bridge_port(dev)))
+		goto out;
+
+	port = br_port_get_check_rcu(dev);
+	if (unlikely(!port || !port->br))
+		goto out;
+
+	dev = __br_fdb_find_port(port->br->dev, opt->addr, opt->vid, true);
+	if (dev)
+		ret = dev->ifindex;
+out:
+	rcu_read_unlock();
+
+	return ret;
+}
+
 struct net_bridge_fdb_entry *br_fdb_find_rcu(struct net_bridge *br,
 					     const unsigned char *addr,
 					     __u16 vid)

net/bridge/br_private.h +5 -0

index 2661dda1a92b..64d4f1727da2 100644
@@ -18,6 +18,7 @@
 #include <linux/if_vlan.h>
 #include <linux/rhashtable.h>
 #include <linux/refcount.h>
+#include <linux/bpf.h>
 
 #define BR_HASH_BITS 8
 #define BR_HASH_SIZE (1 << BR_HASH_BITS)

@@ -2094,4 +2095,15 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 void br_do_suppress_nd(struct sk_buff *skb, struct net_bridge *br,
 		       u16 vid, struct net_bridge_port *p, struct nd_msg *msg);
 struct nd_msg *br_is_nd_neigh_msg(struct sk_buff *skb, struct nd_msg *m);
+
+#define NF_BPF_FDB_OPTS_SZ	12
+struct bpf_fdb_lookup {
+	u8	addr[ETH_ALEN]; /* ETH_ALEN */

net/bridge/br_private.h void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br, · line 2104

Quoted 3 lines

net/bridge/br_private.h +3 -0

+	u16	vid;
+	u32	ifindex;
+};

It seems like addr and ifindex should always be required, right? So why not make them regular function args? That way the ptr to eth addr could be a ptr directly to the packet header (saving a memcpy), and the common case(?) could just pass a NULL opts struct?

net/bridge/br_private.h void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br, · line 2108

Quoted 4 lines

net/bridge/br_private.h +3 -0

+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+				  struct bpf_fdb_lookup *opt,
+				  u32 opt__sz);

It should probably be documented that the return value is an ifindex as well; I guess one of the drawbacks of kfunc's relative to regular helpers is that there is no convention for how to document their usage - maybe we should fix that before we get too many of them? :)

-Toke

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	rcu_read_lock();

This is not needed when the function is only being called from XDP...

don't we need it since we do not hold the rtnl here?

Quoted 4 lines · Lorenzo Bianconi

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	rcu_read_lock();

This is not needed when the function is only being called from XDP...

Quoted 1 line

don't we need it since we do not hold the rtnl here?

No. XDP programs always run under local_bh_disable() which "counts" as an rcu_read_lock(); I did some cleanup around this a while ago, see this commit for a longer explanation:

782347b6bcad ("xdp: Add proper __rcu annotations to redirect map entries")

-Toke

Quoted 17 lines

[ snip to focus on the API ]

net/bridge/br_fdb.c static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br, · line 287

Quoted 12 lines

net/bridge/br_fdb.c +12 -0

+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+				  struct bpf_fdb_lookup *opt,
+				  u32 opt__sz)
+{
+	struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
+	struct net_bridge_port *port;
+	struct net_device *dev;
+	int ret = -ENODEV;
+
+	BUILD_BUG_ON(sizeof(struct bpf_fdb_lookup) != NF_BPF_FDB_OPTS_SZ);
+	if (!opt || opt__sz != sizeof(struct bpf_fdb_lookup))
+		return -ENODEV;

Why is the BUILD_BUG_ON needed? Or why is the NF_BPF_FDB_OPTS_SZ constant even needed?

I added it to be symmetric with respect to ct counterpart

[...]

Quoted 18 lines · Lorenzo Bianconi

Quoted 17 lines

[ snip to focus on the API ]

net/bridge/br_fdb.c static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br, · line 287

Quoted 12 lines

net/bridge/br_fdb.c +12 -0

+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+                           struct bpf_fdb_lookup *opt,
+                           u32 opt__sz)
+{
+ struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
+ struct net_bridge_port *port;
+ struct net_device *dev;
+ int ret = -ENODEV;
+
+ BUILD_BUG_ON(sizeof(struct bpf_fdb_lookup) != NF_BPF_FDB_OPTS_SZ);
+ if (!opt || opt__sz != sizeof(struct bpf_fdb_lookup))
+         return -ENODEV;

Why is the BUILD_BUG_ON needed? Or why is the NF_BPF_FDB_OPTS_SZ constant even needed?

Quoted 1 line

I added it to be symmetric with respect to ct counterpart

But the constant needs to be an enum, not a define, otherwise it will not be emitted to BTF, I added it so that one could easily check the struct 'version' (because sizeof is not relocated in BPF programs).

Yes, bpf_core_field_exists and would also work, but the size is fixed anyway and we need to check it, so it felt better to give it a name and also make it visible to BPF programs at the same time.

Quoted 2 lines

[...]

--
Kartikeya

Similar to bpf_xdp_ct_lookup routine, introduce br_fdb_find_port_from_ifindex unstable helper in order to accelerate linux bridge with XDP. br_fdb_find_port_from_ifindex will perform a lookup in the associated bridge fdb table and it will return the output ifindex if the destination address is associated to a bridge port or -ENODEV for BOM traffic or if lookup fails.

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 net/bridge/br.c         | 21 +++++++++++++
 net/bridge/br_fdb.c     | 67 +++++++++++++++++++++++++++++++++++------
 net/bridge/br_private.h | 12 ++++++++
 3 files changed, 91 insertions(+), 9 deletions(-)

net/bridge/br.c +13 -0

index 1fac72cc617f..d2d1c2341d9c 100644
@@ -16,6 +16,8 @@
 #include <net/llc.h>
 #include <net/stp.h>
 #include <net/switchdev.h>
+#include <linux/btf.h>
+#include <linux/btf_ids.h>
 
 #include "br_private.h"
 

@@ -365,6 +367,17 @@ static const struct stp_proto br_stp_proto = {
 	.rcv	= br_stp_rcv,
 };
 
+#if (IS_ENABLED(CONFIG_DEBUG_INFO_BTF) || IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
+BTF_SET_START(br_xdp_fdb_check_kfunc_ids)
+BTF_ID(func, br_fdb_find_port_from_ifindex)
+BTF_SET_END(br_xdp_fdb_check_kfunc_ids)
+
+static const struct btf_kfunc_id_set br_xdp_fdb_kfunc_set = {
+	.owner     = THIS_MODULE,
+	.check_set = &br_xdp_fdb_check_kfunc_ids,
+};
+#endif
+
 static int __init br_init(void)
 {
 	int err;

net/bridge/br.c +3 -0

+	}
+#endif
+
 	return 0;
 
 err_out6:

net/bridge/br_fdb.c +4 -6

index 6ccda68bd473..cd3afa240298 100644
@@ -235,30 +235,79 @@ static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br,
 	return fdb;
 }
 
-struct net_device *br_fdb_find_port(const struct net_device *br_dev,
-				    const unsigned char *addr,
-				    __u16 vid)
+static struct net_device *
+__br_fdb_find_port(const struct net_device *br_dev,
+		   const unsigned char *addr,
+		   __u16 vid, bool ts_update)
 {
 	struct net_bridge_fdb_entry *f;
-	struct net_device *dev = NULL;
 	struct net_bridge *br;
 
-	ASSERT_RTNL();
-
 	if (!netif_is_bridge_master(br_dev))
 		return NULL;

net/bridge/br_fdb.c +25 -0

+	}
+	return NULL;
+}
+
+struct net_device *br_fdb_find_port(const struct net_device *br_dev,
+				    const unsigned char *addr,
+				    __u16 vid)
+{
+	struct net_device *dev;
+
+	ASSERT_RTNL();
+
+	rcu_read_lock();
+	dev = __br_fdb_find_port(br_dev, addr, vid, false);
 	rcu_read_unlock();
 
 	return dev;
 }
 EXPORT_SYMBOL_GPL(br_fdb_find_port);
 
+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+				  struct bpf_fdb_lookup *opt,
+				  u32 opt__sz)
+{
+	struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
+	struct net_bridge_port *port;
+	struct net_device *dev;
+	int ret = -ENODEV;
+
+	BUILD_BUG_ON(sizeof(struct bpf_fdb_lookup) != NF_BPF_FDB_OPTS_SZ);
+	if (!opt || opt__sz != sizeof(struct bpf_fdb_lookup))

net/bridge/br_fdb.c +14 -0

+
+	port = br_port_get_check_rcu(dev);
+	if (unlikely(!port || !port->br))
+		goto out;
+
+	dev = __br_fdb_find_port(port->br->dev, opt->addr, opt->vid, true);
+	if (dev)
+		ret = dev->ifindex;
+out:
+	rcu_read_unlock();
+
+	return ret;
+}
+
 struct net_bridge_fdb_entry *br_fdb_find_rcu(struct net_bridge *br,
 					     const unsigned char *addr,
 					     __u16 vid)

net/bridge/br_private.h +3 -0

index 2661dda1a92b..64d4f1727da2 100644
@@ -18,6 +18,7 @@
 #include <linux/if_vlan.h>
 #include <linux/rhashtable.h>
 #include <linux/refcount.h>
+#include <linux/bpf.h>
 
 #define BR_HASH_BITS 8
 #define BR_HASH_SIZE (1 << BR_HASH_BITS)

@@ -2094,4 +2095,15 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 void br_do_suppress_nd(struct sk_buff *skb, struct net_bridge *br,
 		       u16 vid, struct net_bridge_port *p, struct nd_msg *msg);
 struct nd_msg *br_is_nd_neigh_msg(struct sk_buff *skb, struct nd_msg *m);
+
+#define NF_BPF_FDB_OPTS_SZ	12

Quoted 11 lines

net/bridge/br_fdb.c +10 -0

+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+                               struct bpf_fdb_lookup *opt,
+                               u32 opt__sz)
+{
+     struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
+     struct net_bridge_port *port;
+     struct net_device *dev;
+     int ret = -ENODEV;
+
+     BUILD_BUG_ON(sizeof(struct bpf_fdb_lookup) != NF_BPF_FDB_OPTS_SZ);

net/bridge/br_fdb.c static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br, · line 293

Quoted 8 lines

net/bridge/br_fdb.c +8 -0

+     if (!opt || opt__sz != sizeof(struct bpf_fdb_lookup))
+             return -ENODEV;
+
+     rcu_read_lock();
+
+     dev = dev_get_by_index_rcu(dev_net(ctx->rxq->dev), opt->ifindex);
+     if (!dev)
+             goto out;

Quoted 11 lines

Quoted 11 lines

net/bridge/br_fdb.c +10 -0

+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+                               struct bpf_fdb_lookup *opt,
+                               u32 opt__sz)
+{
+     struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
+     struct net_bridge_port *port;
+     struct net_device *dev;
+     int ret = -ENODEV;
+
+     BUILD_BUG_ON(sizeof(struct bpf_fdb_lookup) != NF_BPF_FDB_OPTS_SZ);

Quoted 8 lines

net/bridge/br_fdb.c static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br, · line 293

Quoted 8 lines

net/bridge/br_fdb.c +8 -0

+     if (!opt || opt__sz != sizeof(struct bpf_fdb_lookup))
+             return -ENODEV;
+
+     rcu_read_lock();
+
+     dev = dev_get_by_index_rcu(dev_net(ctx->rxq->dev), opt->ifindex);
+     if (!dev)
+             goto out;

imo that is way too much wrapping for an unstable helper. The dev lookup is not cheap.

With all the extra checks the XDP acceleration gets reduced. I think it would be better to use kprobe/fentry on bridge functions that operate on fdb and replicate necessary data into bpf map.
Then xdp prog would do a single cheap lookup from that map to figure out 'port'.

Quoted 24 lines · Nikolay Aleksandrov

Quoted 11 lines

Quoted 11 lines

net/bridge/br_fdb.c +10 -0

+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+                               struct bpf_fdb_lookup *opt,
+                               u32 opt__sz)
+{
+     struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
+     struct net_bridge_port *port;
+     struct net_device *dev;
+     int ret = -ENODEV;
+
+     BUILD_BUG_ON(sizeof(struct bpf_fdb_lookup) != NF_BPF_FDB_OPTS_SZ);

Quoted 8 lines

net/bridge/br_fdb.c static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br, · line 293

Quoted 8 lines

net/bridge/br_fdb.c +8 -0

+     if (!opt || opt__sz != sizeof(struct bpf_fdb_lookup))
+             return -ENODEV;
+
+     rcu_read_lock();
+
+     dev = dev_get_by_index_rcu(dev_net(ctx->rxq->dev), opt->ifindex);
+     if (!dev)
+             goto out;

imo that is way too much wrapping for an unstable helper. The dev lookup is not cheap.

Quoted 6 lines

With all the extra checks the XDP acceleration gets reduced. I think it would be better to use kprobe/fentry on bridge functions that operate on fdb and replicate necessary data into bpf map.
Then xdp prog would do a single cheap lookup from that map to figure out 'port'.

ack, right. This is a very interesting approach. I will investigate it. Thanks.

Quoted 7 lines

Similar to bpf_xdp_ct_lookup routine, introduce br_fdb_find_port_from_ifindex unstable helper in order to accelerate linux bridge with XDP. br_fdb_find_port_from_ifindex will perform a lookup in the associated bridge fdb table and it will return the output ifindex if the destination address is associated to a bridge port or -ENODEV for BOM traffic or if lookup fails.

Quoted 7 lines

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 net/bridge/br.c         | 21 +++++++++++++
 net/bridge/br_fdb.c     | 67 +++++++++++++++++++++++++++++++++++------
 net/bridge/br_private.h | 12 ++++++++
 3 files changed, 91 insertions(+), 9 deletions(-)

Hi Lorenzo,

Please CC bridge maintainers for bridge-related patches, I've added Roopa and the bridge mailing list as well. Aside from that, the change is certainly interesting, I've been thinking about a similar helper for some time now, few comments below.

Have you thought about the egress path and if by the current bridge state the packet would be allowed to egress through the found port from the lookup? I'd guess you have to keep updating the active ports list based on netlink events, but there's a lot of egress bridge logic that either have to be duplicated or somehow synced. Check should_deliver() (br_forward.c) and later egress stages, but I see how this is a good first step and perhaps we can build upon it. There are a few possible solutions, but I haven't tried anything yet, most obvious being yet another helper. :)

Quoted 36 lines

net/bridge/br.c +13 -0

index 1fac72cc617f..d2d1c2341d9c 100644
@@ -16,6 +16,8 @@
 #include <net/llc.h>
 #include <net/stp.h>
 #include <net/switchdev.h>
+#include <linux/btf.h>
+#include <linux/btf_ids.h>
 
 #include "br_private.h"
 

@@ -365,6 +367,17 @@ static const struct stp_proto br_stp_proto = {
 	.rcv	= br_stp_rcv,
 };
 
+#if (IS_ENABLED(CONFIG_DEBUG_INFO_BTF) || IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
+BTF_SET_START(br_xdp_fdb_check_kfunc_ids)
+BTF_ID(func, br_fdb_find_port_from_ifindex)
+BTF_SET_END(br_xdp_fdb_check_kfunc_ids)
+
+static const struct btf_kfunc_id_set br_xdp_fdb_kfunc_set = {
+	.owner     = THIS_MODULE,
+	.check_set = &br_xdp_fdb_check_kfunc_ids,
+};
+#endif
+
 static int __init br_init(void)
 {
 	int err;

@@ -417,6 +430,14 @@ static int __init br_init(void)
 		"need this.\n");
 #endif
 

net/bridge/br.c static int __init br_init(void) · line 437

Quoted 5 lines

net/bridge/br.c +5 -0

+#if (IS_ENABLED(CONFIG_DEBUG_INFO_BTF) || IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
+	err = register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &br_xdp_fdb_kfunc_set);
+	if (err < 0) {
+		br_netlink_fini();
+		goto err_out6;

Add err_out7 and handle it there please. Let's keep it consistent. Also I cannot find register_btf_kfunc_id_set() in net-next or Linus' master, but should it be paired with an unregister on unload (br_deinit) ?

Quoted 32 lines

net/bridge/br.c +3 -0

+	}
+#endif
+
 	return 0;
 
 err_out6:

net/bridge/br_fdb.c +4 -6

index 6ccda68bd473..cd3afa240298 100644
@@ -235,30 +235,79 @@ static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br,
 	return fdb;
 }
 
-struct net_device *br_fdb_find_port(const struct net_device *br_dev,
-				    const unsigned char *addr,
-				    __u16 vid)
+static struct net_device *
+__br_fdb_find_port(const struct net_device *br_dev,
+		   const unsigned char *addr,
+		   __u16 vid, bool ts_update)
 {
 	struct net_bridge_fdb_entry *f;
-	struct net_device *dev = NULL;
 	struct net_bridge *br;
 
-	ASSERT_RTNL();
-
 	if (!netif_is_bridge_master(br_dev))
 		return NULL;
 

net/bridge/br_fdb.c static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br, · line 254

Quoted 9 lines

net/bridge/br_fdb.c +4 -3

 	br = netdev_priv(br_dev);
-	rcu_read_lock();
 	f = br_fdb_find_rcu(br, addr, vid);
-	if (f && f->dst)
-		dev = f->dst->dev;
+
+	if (f && f->dst) {
+		f->updated = jiffies;
+		f->used = f->updated;

This is wrong, f->updated should be set only if anything changed for the fdb. Also you can optimize f->used a little bit if you check if jiffies != current value before setting, you can have millions of packets per sec dirtying that cache line.

Aside from the above, it will change expected behaviour for br_fdb_find_port users (mlxsw, added Ido to CC as well) because it will mark the fdb as active and refresh it which should be done only for the ebpf helper, or might be exported through another helper so ebpf users can decide if they want it updated. There are 2 different use cases and it is not ok for both as we'll start refreshing fdbs that have been inactive for a while and would've expired otherwise.

net/bridge/br_fdb.c static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br, · line 255

Quoted 2 lines

net/bridge/br_fdb.c +1 -0

+		return f->dst->dev;

This is wrong as well, f->dst can become NULL (fdb switched to point to the bridge itself). You should make sure to read f->dst only once and work with the result. I know it's been like that, but it was ok when accessed with rtnl held.

Quoted 36 lines

net/bridge/br_fdb.c +29 -0

+	}
+	return NULL;
+}
+
+struct net_device *br_fdb_find_port(const struct net_device *br_dev,
+				    const unsigned char *addr,
+				    __u16 vid)
+{
+	struct net_device *dev;
+
+	ASSERT_RTNL();
+
+	rcu_read_lock();
+	dev = __br_fdb_find_port(br_dev, addr, vid, false);
 	rcu_read_unlock();
 
 	return dev;
 }
 EXPORT_SYMBOL_GPL(br_fdb_find_port);
 
+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+				  struct bpf_fdb_lookup *opt,
+				  u32 opt__sz)
+{
+	struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
+	struct net_bridge_port *port;
+	struct net_device *dev;
+	int ret = -ENODEV;
+
+	BUILD_BUG_ON(sizeof(struct bpf_fdb_lookup) != NF_BPF_FDB_OPTS_SZ);
+	if (!opt || opt__sz != sizeof(struct bpf_fdb_lookup))
+		return -ENODEV;
+
+	rcu_read_lock();
+

net/bridge/br_fdb.c static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br, · line 296

Quoted 6 lines

net/bridge/br_fdb.c +6 -0

+	dev = dev_get_by_index_rcu(dev_net(ctx->rxq->dev), opt->ifindex);
+	if (!dev)
+		goto out;
+
+	if (unlikely(!netif_is_bridge_port(dev)))
+		goto out;

This check shouldn't be needed if the port checks below succeed.

Quoted 36 lines

net/bridge/br_fdb.c +14 -0

+
+	port = br_port_get_check_rcu(dev);
+	if (unlikely(!port || !port->br))
+		goto out;
+
+	dev = __br_fdb_find_port(port->br->dev, opt->addr, opt->vid, true);
+	if (dev)
+		ret = dev->ifindex;
+out:
+	rcu_read_unlock();
+
+	return ret;
+}
+
 struct net_bridge_fdb_entry *br_fdb_find_rcu(struct net_bridge *br,
 					     const unsigned char *addr,
 					     __u16 vid)

net/bridge/br_private.h +3 -0

index 2661dda1a92b..64d4f1727da2 100644
@@ -18,6 +18,7 @@
 #include <linux/if_vlan.h>
 #include <linux/rhashtable.h>
 #include <linux/refcount.h>
+#include <linux/bpf.h>
 
 #define BR_HASH_BITS 8
 #define BR_HASH_SIZE (1 << BR_HASH_BITS)

@@ -2094,4 +2095,15 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 void br_do_suppress_nd(struct sk_buff *skb, struct net_bridge *br,
 		       u16 vid, struct net_bridge_port *p, struct nd_msg *msg);
 struct nd_msg *br_is_nd_neigh_msg(struct sk_buff *skb, struct nd_msg *m);
+
+#define NF_BPF_FDB_OPTS_SZ	12

net/bridge/br_private.h void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br, · line 2109

Quoted 10 lines

net/bridge/br_private.h +9 -0

+struct bpf_fdb_lookup {
+	u8	addr[ETH_ALEN]; /* ETH_ALEN */
+	u16	vid;
+	u32	ifindex;
+};
+
+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+				  struct bpf_fdb_lookup *opt,
+				  u32 opt__sz);
 #endif

Thanks,
Nik

Quoted 16 lines · Lorenzo Bianconi

Quoted 7 lines

Similar to bpf_xdp_ct_lookup routine, introduce br_fdb_find_port_from_ifindex unstable helper in order to accelerate linux bridge with XDP. br_fdb_find_port_from_ifindex will perform a lookup in the associated bridge fdb table and it will return the output ifindex if the destination address is associated to a bridge port or -ENODEV for BOM traffic or if lookup fails.

Quoted 7 lines

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 net/bridge/br.c         | 21 +++++++++++++
 net/bridge/br_fdb.c     | 67 +++++++++++++++++++++++++++++++++++------
 net/bridge/br_private.h | 12 ++++++++
 3 files changed, 91 insertions(+), 9 deletions(-)

Quoted 1 line

Hi Lorenzo,

Hi Nikolay,

thx for the review.

[...]

I guess at the time I sent the series it was just in bpf-next but now it should be in net-next too.
I do not think we need a unregister here.
@Kumar: agree?

Quoted 1 line

[...]

Quoted 16 lines · Lorenzo Bianconi

Quoted 7 lines

Similar to bpf_xdp_ct_lookup routine, introduce br_fdb_find_port_from_ifindex unstable helper in order to accelerate linux bridge with XDP. br_fdb_find_port_from_ifindex will perform a lookup in the associated bridge fdb table and it will return the output ifindex if the destination address is associated to a bridge port or -ENODEV for BOM traffic or if lookup fails.

Quoted 7 lines

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 net/bridge/br.c         | 21 +++++++++++++
 net/bridge/br_fdb.c     | 67 +++++++++++++++++++++++++++++++++++------
 net/bridge/br_private.h | 12 ++++++++
 3 files changed, 91 insertions(+), 9 deletions(-)

Quoted 1 line

Hi Lorenzo,

Hi Nikolay,

thx for the review.

Quoted 3 lines

Please CC bridge maintainers for bridge-related patches, I've added Roopa and the bridge mailing list as well. Aside from that, the change is certainly interesting, I've been thinking about a similar helper for some time now, few comments below.

yes, sorry for that. I figured it out after sending the series out.

Quoted 8 lines

Have you thought about the egress path and if by the current bridge state the packet would be allowed to egress through the found port from the lookup? I'd guess you have to keep updating the active ports list based on netlink events, but there's a lot of egress bridge logic that either have to be duplicated or somehow synced. Check should_deliver() (br_forward.c) and later egress stages, but I see how this is a good first step and perhaps we can build upon it. There are a few possible solutions, but I haven't tried anything yet, most obvious being yet another helper. :)

ack, right but I am bit worried about adding too much logic and slow down xdp performances. I guess we can investigate first the approach proposed by Alexei and then revaluate. Agree?

Quoted 36 lines

Quoted 36 lines

net/bridge/br.c +13 -0

index 1fac72cc617f..d2d1c2341d9c 100644
@@ -16,6 +16,8 @@
 #include <net/llc.h>
 #include <net/stp.h>
 #include <net/switchdev.h>
+#include <linux/btf.h>
+#include <linux/btf_ids.h>
 
 #include "br_private.h"
 

@@ -365,6 +367,17 @@ static const struct stp_proto br_stp_proto = {
 	.rcv	= br_stp_rcv,
 };
 
+#if (IS_ENABLED(CONFIG_DEBUG_INFO_BTF) || IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
+BTF_SET_START(br_xdp_fdb_check_kfunc_ids)
+BTF_ID(func, br_fdb_find_port_from_ifindex)
+BTF_SET_END(br_xdp_fdb_check_kfunc_ids)
+
+static const struct btf_kfunc_id_set br_xdp_fdb_kfunc_set = {
+	.owner     = THIS_MODULE,
+	.check_set = &br_xdp_fdb_check_kfunc_ids,
+};
+#endif
+
 static int __init br_init(void)
 {
 	int err;

@@ -417,6 +430,14 @@ static int __init br_init(void)
 		"need this.\n");
 #endif
 

Quoted 9 lines

net/bridge/br.c static int __init br_init(void) · line 437

Quoted 5 lines

net/bridge/br.c +5 -0

+#if (IS_ENABLED(CONFIG_DEBUG_INFO_BTF) || IS_ENABLED(CONFIG_DEBUG_INFO_BTF_MODULES))
+	err = register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &br_xdp_fdb_kfunc_set);
+	if (err < 0) {
+		br_netlink_fini();
+		goto err_out6;

Add err_out7 and handle it there please. Let's keep it consistent. Also I cannot find register_btf_kfunc_id_set() in net-next or Linus' master, but should it be paired with an unregister on unload (br_deinit) ?

I guess at the time I sent the series it was just in bpf-next but now it should be in net-next too.
I do not think we need a unregister here.
@Kumar: agree?

Quoted 32 lines

Quoted 32 lines

net/bridge/br.c +3 -0

+	}
+#endif
+
 	return 0;
 
 err_out6:

net/bridge/br_fdb.c +4 -6

index 6ccda68bd473..cd3afa240298 100644
@@ -235,30 +235,79 @@ static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br,
 	return fdb;
 }
 
-struct net_device *br_fdb_find_port(const struct net_device *br_dev,
-				    const unsigned char *addr,
-				    __u16 vid)
+static struct net_device *
+__br_fdb_find_port(const struct net_device *br_dev,
+		   const unsigned char *addr,
+		   __u16 vid, bool ts_update)
 {
 	struct net_bridge_fdb_entry *f;
-	struct net_device *dev = NULL;
 	struct net_bridge *br;
 
-	ASSERT_RTNL();
-
 	if (!netif_is_bridge_master(br_dev))
 		return NULL;
 

Quoted 13 lines

net/bridge/br_fdb.c static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br, · line 254

Quoted 9 lines

net/bridge/br_fdb.c +4 -3

 	br = netdev_priv(br_dev);
-	rcu_read_lock();
 	f = br_fdb_find_rcu(br, addr, vid);
-	if (f && f->dst)
-		dev = f->dst->dev;
+
+	if (f && f->dst) {
+		f->updated = jiffies;
+		f->used = f->updated;

This is wrong, f->updated should be set only if anything changed for the fdb. Also you can optimize f->used a little bit if you check if jiffies != current value before setting, you can have millions of packets per sec dirtying that cache line.

ack, right. I will fix it.

Quoted 7 lines

Aside from the above, it will change expected behaviour for br_fdb_find_port users (mlxsw, added Ido to CC as well) because it will mark the fdb as active and refresh it which should be done only for the ebpf helper, or might be exported through another helper so ebpf users can decide if they want it updated. There are 2 different use cases and it is not ok for both as we'll start refreshing fdbs that have been inactive for a while and would've expired otherwise.

This is a bug actually. I forgot to check ts_update in the if condition, something like:

if (f && f->dst && ts_update) {
...
}

Quoted 6 lines

net/bridge/br_fdb.c static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br, · line 255

Quoted 2 lines

net/bridge/br_fdb.c +1 -0

+		return f->dst->dev;

This is wrong as well, f->dst can become NULL (fdb switched to point to the bridge itself). You should make sure to read f->dst only once and work with the result. I know it's been like that, but it was ok when accessed with rtnl held.

uhm, right. I will fix it.

Quoted 36 lines

Quoted 36 lines

net/bridge/br_fdb.c +29 -0

+	}
+	return NULL;
+}
+
+struct net_device *br_fdb_find_port(const struct net_device *br_dev,
+				    const unsigned char *addr,
+				    __u16 vid)
+{
+	struct net_device *dev;
+
+	ASSERT_RTNL();
+
+	rcu_read_lock();
+	dev = __br_fdb_find_port(br_dev, addr, vid, false);
 	rcu_read_unlock();
 
 	return dev;
 }
 EXPORT_SYMBOL_GPL(br_fdb_find_port);
 
+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+				  struct bpf_fdb_lookup *opt,
+				  u32 opt__sz)
+{
+	struct xdp_buff *ctx = (struct xdp_buff *)xdp_ctx;
+	struct net_bridge_port *port;
+	struct net_device *dev;
+	int ret = -ENODEV;
+
+	BUILD_BUG_ON(sizeof(struct bpf_fdb_lookup) != NF_BPF_FDB_OPTS_SZ);
+	if (!opt || opt__sz != sizeof(struct bpf_fdb_lookup))
+		return -ENODEV;
+
+	rcu_read_lock();
+

Quoted 8 lines

net/bridge/br_fdb.c static struct net_bridge_fdb_entry *br_fdb_find(struct net_bridge *br, · line 296

Quoted 6 lines

net/bridge/br_fdb.c +6 -0

+	dev = dev_get_by_index_rcu(dev_net(ctx->rxq->dev), opt->ifindex);
+	if (!dev)
+		goto out;
+
+	if (unlikely(!netif_is_bridge_port(dev)))
+		goto out;

This check shouldn't be needed if the port checks below succeed.

ack, I will fix it.

Regards,
Lorenzo

Quoted 49 lines

Quoted 36 lines

net/bridge/br_fdb.c +14 -0

+
+	port = br_port_get_check_rcu(dev);
+	if (unlikely(!port || !port->br))
+		goto out;
+
+	dev = __br_fdb_find_port(port->br->dev, opt->addr, opt->vid, true);
+	if (dev)
+		ret = dev->ifindex;
+out:
+	rcu_read_unlock();
+
+	return ret;
+}
+
 struct net_bridge_fdb_entry *br_fdb_find_rcu(struct net_bridge *br,
 					     const unsigned char *addr,
 					     __u16 vid)

net/bridge/br_private.h +3 -0

index 2661dda1a92b..64d4f1727da2 100644
@@ -18,6 +18,7 @@
 #include <linux/if_vlan.h>
 #include <linux/rhashtable.h>
 #include <linux/refcount.h>
+#include <linux/bpf.h>
 
 #define BR_HASH_BITS 8
 #define BR_HASH_SIZE (1 << BR_HASH_BITS)

@@ -2094,4 +2095,15 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 void br_do_suppress_nd(struct sk_buff *skb, struct net_bridge *br,
 		       u16 vid, struct net_bridge_port *p, struct nd_msg *msg);
 struct nd_msg *br_is_nd_neigh_msg(struct sk_buff *skb, struct nd_msg *m);
+
+#define NF_BPF_FDB_OPTS_SZ	12

net/bridge/br_private.h void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br, · line 2109

Quoted 10 lines

net/bridge/br_private.h +9 -0

+struct bpf_fdb_lookup {
+	u8	addr[ETH_ALEN]; /* ETH_ALEN */
+	u16	vid;
+	u32	ifindex;
+};
+
+int br_fdb_find_port_from_ifindex(struct xdp_md *xdp_ctx,
+				  struct bpf_fdb_lookup *opt,
+				  u32 opt__sz);
 #endif

Thanks,
Nik

Quoted 37 lines · Lorenzo Bianconi

Quoted 16 lines · Lorenzo Bianconi

Quoted 7 lines

Similar to bpf_xdp_ct_lookup routine, introduce br_fdb_find_port_from_ifindex unstable helper in order to accelerate linux bridge with XDP. br_fdb_find_port_from_ifindex will perform a lookup in the associated bridge fdb table and it will return the output ifindex if the destination address is associated to a bridge port or -ENODEV for BOM traffic or if lookup fails.

Quoted 7 lines

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 net/bridge/br.c         | 21 +++++++++++++
 net/bridge/br_fdb.c     | 67 +++++++++++++++++++++++++++++++++++------
 net/bridge/br_private.h | 12 ++++++++
 3 files changed, 91 insertions(+), 9 deletions(-)

Quoted 1 line

Hi Lorenzo,

Hi Nikolay,

thx for the review.

Quoted 3 lines

Please CC bridge maintainers for bridge-related patches, I've added Roopa and the bridge mailing list as well. Aside from that, the change is certainly interesting, I've been thinking about a similar helper for some time now, few comments below.

yes, sorry for that. I figured it out after sending the series out.

Quoted 8 lines

Have you thought about the egress path and if by the current bridge state the packet would be allowed to egress through the found port from the lookup? I'd guess you have to keep updating the active ports list based on netlink events, but there's a lot of egress bridge logic that either have to be duplicated or somehow synced. Check should_deliver() (br_forward.c) and later egress stages, but I see how this is a good first step and perhaps we can build upon it. There are a few possible solutions, but I haven't tried anything yet, most obvious being yet another helper. :)

Quoted 4 lines

ack, right but I am bit worried about adding too much logic and slow down xdp performances. I guess we can investigate first the approach proposed by Alexei and then revaluate. Agree?

Sure, that approach sounds very interesting, but my point was that bypassing the ingress and egress logic defeats most of the bridge features. You just get an fdb hash table which you can build today with ebpf without any changes to the kernel. :) You have multiple states, flags and options for each port and each vlan which can change dynamically based on external events (e.g. STP, config changes etc) and they can affect forwarding even if the fdbs remain in the table.

Quoted 43 lines · Nikolay Aleksandrov

Quoted 37 lines · Lorenzo Bianconi

Quoted 16 lines · Lorenzo Bianconi

Quoted 7 lines

Similar to bpf_xdp_ct_lookup routine, introduce br_fdb_find_port_from_ifindex unstable helper in order to accelerate linux bridge with XDP. br_fdb_find_port_from_ifindex will perform a lookup in the associated bridge fdb table and it will return the output ifindex if the destination address is associated to a bridge port or -ENODEV for BOM traffic or if lookup fails.

Quoted 7 lines

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 net/bridge/br.c         | 21 +++++++++++++
 net/bridge/br_fdb.c     | 67 +++++++++++++++++++++++++++++++++++------
 net/bridge/br_private.h | 12 ++++++++
 3 files changed, 91 insertions(+), 9 deletions(-)

Quoted 1 line

Hi Lorenzo,

Hi Nikolay,

thx for the review.

Quoted 3 lines

Please CC bridge maintainers for bridge-related patches, I've added Roopa and the bridge mailing list as well. Aside from that, the change is certainly interesting, I've been thinking about a similar helper for some time now, few comments below.

yes, sorry for that. I figured it out after sending the series out.

Quoted 8 lines

Have you thought about the egress path and if by the current bridge state the packet would be allowed to egress through the found port from the lookup? I'd guess you have to keep updating the active ports list based on netlink events, but there's a lot of egress bridge logic that either have to be duplicated or somehow synced. Check should_deliver() (br_forward.c) and later egress stages, but I see how this is a good first step and perhaps we can build upon it. There are a few possible solutions, but I haven't tried anything yet, most obvious being yet another helper. :)

Quoted 4 lines

ack, right but I am bit worried about adding too much logic and slow down xdp performances. I guess we can investigate first the approach proposed by Alexei and then revaluate. Agree?

Quoted 7 lines

Sure, that approach sounds very interesting, but my point was that bypassing the ingress and egress logic defeats most of the bridge features. You just get an fdb hash table which you can build today with ebpf without any changes to the kernel. :) You have multiple states, flags and options for each port and each vlan which can change dynamically based on external events (e.g. STP, config changes etc) and they can affect forwarding even if the fdbs remain in the table.

To me, leveraging all this is precisely the reason to have BPF helpers instead of just replicating state in BPF maps: it's very easy to do that and show a nice speedup, and then once you get all the corner cases covered that the in-kernel code already deals with, you've chipped away at that speedup and spent a lot of time essentially re-writing the battle-tested code already in the kernel.

So I think figuring out how to do the state sync is the right thing to do; a second helper would be fine for this, IMO, but I'm not really familiar enough with the bridge code to really have a qualified opinion.

-Toke

Quoted 64 lines

Quoted 43 lines · Nikolay Aleksandrov

Quoted 37 lines · Lorenzo Bianconi

Quoted 16 lines · Lorenzo Bianconi

Quoted 7 lines

Similar to bpf_xdp_ct_lookup routine, introduce br_fdb_find_port_from_ifindex unstable helper in order to accelerate linux bridge with XDP. br_fdb_find_port_from_ifindex will perform a lookup in the associated bridge fdb table and it will return the output ifindex if the destination address is associated to a bridge port or -ENODEV for BOM traffic or if lookup fails.

Quoted 7 lines

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 net/bridge/br.c         | 21 +++++++++++++
 net/bridge/br_fdb.c     | 67 +++++++++++++++++++++++++++++++++++------
 net/bridge/br_private.h | 12 ++++++++
 3 files changed, 91 insertions(+), 9 deletions(-)

Quoted 1 line

Hi Lorenzo,

Hi Nikolay,

thx for the review.

Quoted 3 lines

Please CC bridge maintainers for bridge-related patches, I've added Roopa and the bridge mailing list as well. Aside from that, the change is certainly interesting, I've been thinking about a similar helper for some time now, few comments below.

yes, sorry for that. I figured it out after sending the series out.

Quoted 8 lines

Have you thought about the egress path and if by the current bridge state the packet would be allowed to egress through the found port from the lookup? I'd guess you have to keep updating the active ports list based on netlink events, but there's a lot of egress bridge logic that either have to be duplicated or somehow synced. Check should_deliver() (br_forward.c) and later egress stages, but I see how this is a good first step and perhaps we can build upon it. There are a few possible solutions, but I haven't tried anything yet, most obvious being yet another helper. :)

Quoted 4 lines

ack, right but I am bit worried about adding too much logic and slow down xdp performances. I guess we can investigate first the approach proposed by Alexei and then revaluate. Agree?

Quoted 7 lines

Sure, that approach sounds very interesting, but my point was that bypassing the ingress and egress logic defeats most of the bridge features. You just get an fdb hash table which you can build today with ebpf without any changes to the kernel. :) You have multiple states, flags and options for each port and each vlan which can change dynamically based on external events (e.g. STP, config changes etc) and they can affect forwarding even if the fdbs remain in the table.

To me, leveraging all this is precisely the reason to have BPF helpers instead of just replicating state in BPF maps: it's very easy to do that and show a nice speedup, and then once you get all the corner cases covered that the in-kernel code already deals with, you've chipped away at that speedup and spent a lot of time essentially re-writing the battle-tested code already in the kernel.

So I think figuring out how to do the state sync is the right thing to do; a second helper would be fine for this, IMO, but I'm not really familiar enough with the bridge code to really have a qualified opinion.

Quoted 2 lines

-Toke

Right, sounds good to me. IMO it should be required in order to get a meaningful bridge speedup, otherwise the solution is incomplete and you just do simple lookups that ignore all of the state that could impact the forwarding decision.

Cheers,
Nik