[RFC bpf-next 0/2] introduce bpf fdb lookup helper for xdp

[RFC bpf-next 0/2] introduce bpf fdb lookup helper for xdp

[Lorenzo Bianconi]

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.

Lorenzo Bianconi (2):
  net: bridge: add unstable br_fdb_find_port_from_ifindex helper
  samples: bpf: add xdp fdb lookup program

 net/bridge/br.c            |  21 +++++
 net/bridge/br_fdb.c        |  67 +++++++++++++---
 net/bridge/br_private.h    |  12 +++
 samples/bpf/Makefile       |   9 ++-
 samples/bpf/xdp_fdb.bpf.c  |  68 +++++++++++++++++
 samples/bpf/xdp_fdb_user.c | 152 +++++++++++++++++++++++++++++++++++++
 6 files changed, 319 insertions(+), 10 deletions(-)
 create mode 100644 samples/bpf/xdp_fdb.bpf.c
 create mode 100644 samples/bpf/xdp_fdb_user.c

-- 
2.34.1

[RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Lorenzo Bianconi]

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 <lorenzo@kernel.org>
---
 net/bridge/br.c         | 21 +++++++++++++
 net/bridge/br_fdb.c     | 67 +++++++++++++++++++++++++++++++++++------
 net/bridge/br_private.h | 12 ++++++++
 3 files changed, 91 insertions(+), 9 deletions(-)

diff --git a/net/bridge/br.c b/net/bridge/br.c
index 1fac72cc617f..d2d1c2341d9c 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -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
 
+#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;
+	}
+#endif
+
 	return 0;
 
 err_out6:
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 6ccda68bd473..cd3afa240298 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -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;
 
 	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;
+		return f->dst->dev;
+	}
+	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();
+
+	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)
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 2661dda1a92b..64d4f1727da2 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -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 */
+	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
-- 
2.34.1

[RFC bpf-next 2/2] samples: bpf: add xdp fdb lookup program

[Lorenzo Bianconi]

This patch adds an example of a xdp-based bridge with the new
br_fdb_find_port_from_ifindex unstable helper.
This program simply forwards packets based on the destination address
running a fdb lookup on a bridge fdb table in the kernel.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 samples/bpf/Makefile       |   9 ++-
 samples/bpf/xdp_fdb.bpf.c  |  68 +++++++++++++++++
 samples/bpf/xdp_fdb_user.c | 152 +++++++++++++++++++++++++++++++++++++
 3 files changed, 228 insertions(+), 1 deletion(-)
 create mode 100644 samples/bpf/xdp_fdb.bpf.c
 create mode 100644 samples/bpf/xdp_fdb_user.c

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 38638845db9d..1fb4544a66a7 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -59,6 +59,7 @@ tprogs-y += xdp_redirect_map_multi
 tprogs-y += xdp_redirect_map
 tprogs-y += xdp_redirect
 tprogs-y += xdp_monitor
+tprogs-y += xdp_fdb
 
 # Libbpf dependencies
 LIBBPF_SRC = $(TOOLS_PATH)/lib/bpf
@@ -124,6 +125,7 @@ xdp_redirect_cpu-objs := xdp_redirect_cpu_user.o $(XDP_SAMPLE)
 xdp_redirect_map-objs := xdp_redirect_map_user.o $(XDP_SAMPLE)
 xdp_redirect-objs := xdp_redirect_user.o $(XDP_SAMPLE)
 xdp_monitor-objs := xdp_monitor_user.o $(XDP_SAMPLE)
+xdp_fdb-objs := xdp_fdb_user.o $(XDP_SAMPLE)
 
 # Tell kbuild to always build the programs
 always-y := $(tprogs-y)
@@ -226,6 +228,7 @@ TPROGLDLIBS_map_perf_test	+= -lrt
 TPROGLDLIBS_test_overhead	+= -lrt
 TPROGLDLIBS_xdpsock		+= -pthread -lcap
 TPROGLDLIBS_xsk_fwd		+= -pthread
+TPROGLDLIBS_xdp_fdb		+= -lm
 
 # Allows pointing LLC/CLANG to a LLVM backend with bpf support, redefine on cmdline:
 # make M=samples/bpf LLC=~/git/llvm-project/llvm/build/bin/llc CLANG=~/git/llvm-project/llvm/build/bin/clang
@@ -342,6 +345,7 @@ $(obj)/xdp_redirect_map_multi_user.o: $(obj)/xdp_redirect_map_multi.skel.h
 $(obj)/xdp_redirect_map_user.o: $(obj)/xdp_redirect_map.skel.h
 $(obj)/xdp_redirect_user.o: $(obj)/xdp_redirect.skel.h
 $(obj)/xdp_monitor_user.o: $(obj)/xdp_monitor.skel.h
+$(obj)/xdp_fdb_user.o: $(obj)/xdp_fdb.skel.h
 
 $(obj)/tracex5_kern.o: $(obj)/syscall_nrs.h
 $(obj)/hbm_out_kern.o: $(src)/hbm.h $(src)/hbm_kern.h
@@ -399,6 +403,7 @@ $(obj)/xdp_redirect_map_multi.bpf.o: $(obj)/xdp_sample.bpf.o
 $(obj)/xdp_redirect_map.bpf.o: $(obj)/xdp_sample.bpf.o
 $(obj)/xdp_redirect.bpf.o: $(obj)/xdp_sample.bpf.o
 $(obj)/xdp_monitor.bpf.o: $(obj)/xdp_sample.bpf.o
+$(obj)/xdp_fdb.bpf.o: $(obj)/xdp_sample.bpf.o
 
 $(obj)/%.bpf.o: $(src)/%.bpf.c $(obj)/vmlinux.h $(src)/xdp_sample.bpf.h $(src)/xdp_sample_shared.h
 	@echo "  CLANG-BPF " $@
@@ -409,7 +414,8 @@ $(obj)/%.bpf.o: $(src)/%.bpf.c $(obj)/vmlinux.h $(src)/xdp_sample.bpf.h $(src)/x
 		-c $(filter %.bpf.c,$^) -o $@
 
 LINKED_SKELS := xdp_redirect_cpu.skel.h xdp_redirect_map_multi.skel.h \
-		xdp_redirect_map.skel.h xdp_redirect.skel.h xdp_monitor.skel.h
+		xdp_redirect_map.skel.h xdp_redirect.skel.h xdp_monitor.skel.h \
+		xdp_fdb.skel.h
 clean-files += $(LINKED_SKELS)
 
 xdp_redirect_cpu.skel.h-deps := xdp_redirect_cpu.bpf.o xdp_sample.bpf.o
@@ -417,6 +423,7 @@ xdp_redirect_map_multi.skel.h-deps := xdp_redirect_map_multi.bpf.o xdp_sample.bp
 xdp_redirect_map.skel.h-deps := xdp_redirect_map.bpf.o xdp_sample.bpf.o
 xdp_redirect.skel.h-deps := xdp_redirect.bpf.o xdp_sample.bpf.o
 xdp_monitor.skel.h-deps := xdp_monitor.bpf.o xdp_sample.bpf.o
+xdp_fdb.skel.h-deps := xdp_fdb.bpf.o xdp_sample.bpf.o
 
 LINKED_BPF_SRCS := $(patsubst %.bpf.o,%.bpf.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps)))
 
diff --git a/samples/bpf/xdp_fdb.bpf.c b/samples/bpf/xdp_fdb.bpf.c
new file mode 100644
index 000000000000..7c797bfb7300
--- /dev/null
+++ b/samples/bpf/xdp_fdb.bpf.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+#define KBUILD_MODNAME "foo"
+
+#include "vmlinux.h"
+#include "xdp_sample.bpf.h"
+#include "xdp_sample_shared.h"
+
+struct {
+	__uint(type, BPF_MAP_TYPE_DEVMAP_HASH);
+	__uint(key_size, sizeof(int));
+	__uint(value_size, sizeof(int));
+	__uint(max_entries, 64);
+} br_ports SEC(".maps");
+
+struct bpf_fdb_lookup {
+	__u8	addr[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) __ksym;
+
+SEC("xdp")
+int xdp_fdb_lookup(struct xdp_md *ctx)
+{
+	void *data_end = (void *)(long)ctx->data_end;
+	void *data = (void *)(long)ctx->data;
+	u32 key = bpf_get_smp_processor_id();
+	struct bpf_fdb_lookup params = {
+		.ifindex = ctx->ingress_ifindex,
+	};
+	struct ethhdr *eth = data;
+	u64 nh_off = sizeof(*eth);
+	struct datarec *rec;
+	int ret;
+
+	if (data + nh_off > data_end)
+		return XDP_DROP;
+
+	rec = bpf_map_lookup_elem(&rx_cnt, &key);
+	if (!rec)
+		return XDP_PASS;
+
+	NO_TEAR_INC(rec->processed);
+
+	__builtin_memcpy(params.addr, eth->h_dest, ETH_ALEN);
+	ret = br_fdb_find_port_from_ifindex(ctx, &params,
+					    sizeof(struct bpf_fdb_lookup));
+	if (ret < 0)
+		/* In cases of flooding, XDP_PASS will be returned here */
+		return XDP_PASS;
+
+	return bpf_redirect_map(&br_ports, ret, 0);
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/samples/bpf/xdp_fdb_user.c b/samples/bpf/xdp_fdb_user.c
new file mode 100644
index 000000000000..c3bc073f273d
--- /dev/null
+++ b/samples/bpf/xdp_fdb_user.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+static const char *__doc__ =
+"XDP fdb lookup tool, using BPF_MAP_TYPE_DEVMAP\n"
+"Usage: xdp_fdb <IFINDEX_0> <IFINDEX_1> ... <IFINDEX_n>\n";
+
+#include <linux/bpf.h>
+#include <linux/if_link.h>
+#include <assert.h>
+#include <errno.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+#include <net/if.h>
+#include <unistd.h>
+#include <libgen.h>
+#include <getopt.h>
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+#include "bpf_util.h"
+#include "xdp_sample_user.h"
+#include "xdp_fdb.skel.h"
+
+static int mask = SAMPLE_RX_CNT | SAMPLE_REDIRECT_ERR_MAP_CNT |
+		  SAMPLE_EXCEPTION_CNT | SAMPLE_DEVMAP_XMIT_CNT_MULTI |
+		  SAMPLE_REDIRECT_MAP_CNT;
+
+DEFINE_SAMPLE_INIT(xdp_fdb);
+
+static const struct option long_options[] = {
+	{ "help", no_argument, NULL, 'h' },
+	{ "force", no_argument, NULL, 'F' },
+	{ "interval", required_argument, NULL, 'i' },
+	{ "verbose", no_argument, NULL, 'v' },
+	{}
+};
+
+#define IFINDEX_LIST_SZ	32
+static int ifindex_list[IFINDEX_LIST_SZ];
+static int ifindex_num;
+
+int main(int argc, char **argv)
+{
+	int i, opt, ret = EXIT_FAIL_OPTION;
+	bool error = true, force = false;
+	unsigned long interval = 2;
+	struct xdp_fdb *skel;
+
+	while ((opt = getopt_long(argc, argv, "hFi:v",
+				  long_options, NULL)) != -1) {
+		switch (opt) {
+		case 'F':
+			force = true;
+			break;
+		case 'i':
+			interval = strtoul(optarg, NULL, 0);
+			break;
+		case 'v':
+			sample_switch_mode();
+			break;
+		case 'h':
+			error = false;
+		default:
+			sample_usage(argv, long_options, __doc__, mask, error);
+			return ret;
+		}
+	}
+
+	if (argc <= optind + 1) {
+		sample_usage(argv, long_options, __doc__, mask, true);
+		goto end;
+	}
+
+	for (i = optind; i < argc && i < IFINDEX_LIST_SZ; i++) {
+		int index;
+
+		index = if_nametoindex(argv[i]);
+
+		if (!index)
+			index = strtoul(argv[i], NULL, 0);
+		if (index)
+			ifindex_list[ifindex_num++] = index;
+	}
+
+	if (!ifindex_num) {
+		fprintf(stderr, "Bad interface index or name\n");
+		sample_usage(argv, long_options, __doc__, mask, true);
+		goto end;
+	}
+
+	skel = xdp_fdb__open();
+	if (!skel) {
+		fprintf(stderr, "Failed to xdp_fdb__open: %s\n",
+			strerror(errno));
+		ret = EXIT_FAIL_BPF;
+		goto end;
+	}
+
+	ret = sample_init_pre_load(skel);
+	if (ret < 0) {
+		fprintf(stderr, "Failed to sample_init_pre_load: %s\n", strerror(-ret));
+		ret = EXIT_FAIL_BPF;
+		goto end_destroy;
+	}
+
+	ret = xdp_fdb__load(skel);
+	if (ret < 0) {
+		fprintf(stderr, "Failed to xdp_fdb__load: %s\n",
+			strerror(errno));
+		ret = EXIT_FAIL_BPF;
+		goto end_destroy;
+	}
+
+	ret = sample_init(skel, mask);
+	if (ret < 0) {
+		fprintf(stderr, "Failed to initialize sample: %s\n", strerror(-ret));
+		ret = EXIT_FAIL;
+		goto end_destroy;
+	}
+
+	for (i = 0; i < ifindex_num; i++) {
+		if (sample_install_xdp(skel->progs.xdp_fdb_lookup,
+				       ifindex_list[i], false, force) < 0) {
+			ret = EXIT_FAIL_XDP;
+			goto end_destroy;
+		}
+
+		if (bpf_map_update_elem(bpf_map__fd(skel->maps.br_ports),
+					&ifindex_list[i],
+					&ifindex_list[i], 0) < 0) {
+			fprintf(stderr, "Failed to update devmap value: %s\n",
+				strerror(errno));
+			ret = EXIT_FAIL_BPF;
+			goto end_destroy;
+		}
+	}
+
+	ret = sample_run(interval, NULL, NULL);
+	if (ret < 0) {
+		fprintf(stderr, "Failed during sample run: %s\n", strerror(-ret));
+		ret = EXIT_FAIL;
+		goto end_destroy;
+	}
+	ret = EXIT_OK;
+
+end_destroy:
+	xdp_fdb__destroy(skel);
+end:
+	sample_exit(ret);
+}
-- 
2.34.1

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Toke Høiland-Jørgensen]

[ snip to focus on the API ]

> +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?

> +	rcu_read_lock();

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

> +
> +	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)
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 2661dda1a92b..64d4f1727da2 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -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 */
> +	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?

> +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

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Nikolay Aleksandrov]

On 24/01/2022 19:20, Lorenzo Bianconi wrote:
> 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 <lorenzo@kernel.org>
> ---
>  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. :)

> diff --git a/net/bridge/br.c b/net/bridge/br.c
> index 1fac72cc617f..d2d1c2341d9c 100644
> --- a/net/bridge/br.c
> +++ b/net/bridge/br.c
> @@ -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
>  
> +#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) ?

> +	}
> +#endif
> +
>  	return 0;
>  
>  err_out6:
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 6ccda68bd473..cd3afa240298 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -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;
>  
>  	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.

> +		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.

> +	}
> +	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();
> +
> +	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.

> +
> +	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)
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 2661dda1a92b..64d4f1727da2 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -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 */
> +	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

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Alexei Starovoitov]

On Mon, Jan 24, 2022 at 10:32 AM Nikolay Aleksandrov <nikolay@nvidia.com> wrote:
> >
> > +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();
> > +
> > +     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'.

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

> On Mon, Jan 24, 2022 at 10:32 AM Nikolay Aleksandrov <nikolay@nvidia.com> wrote:
> > >
> > > +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();
> > > +
> > > +     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'.

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

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 8810 bytes --]

> On 24/01/2022 19:20, Lorenzo Bianconi wrote:
> > 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 <lorenzo@kernel.org>
> > ---
> >  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,

Hi Nikolay,

thx for the review.

> 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.

> 
> 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?

> 
> > diff --git a/net/bridge/br.c b/net/bridge/br.c
> > index 1fac72cc617f..d2d1c2341d9c 100644
> > --- a/net/bridge/br.c
> > +++ b/net/bridge/br.c
> > @@ -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
> >  
> > +#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?

> 
> > +	}
> > +#endif
> > +
> >  	return 0;
> >  
> >  err_out6:
> > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> > index 6ccda68bd473..cd3afa240298 100644
> > --- a/net/bridge/br_fdb.c
> > +++ b/net/bridge/br_fdb.c
> > @@ -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;
> >  
> >  	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.

> 
> 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) {
 ...
 }

> 
> > +		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.

> 
> > +	}
> > +	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();
> > +
> > +	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

> 
> > +
> > +	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)
> > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> > index 2661dda1a92b..64d4f1727da2 100644
> > --- a/net/bridge/br_private.h
> > +++ b/net/bridge/br_private.h
> > @@ -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 */
> > +	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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3166 bytes --]

> [ snip to focus on the API ]
> 
> > +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

> 
> > +	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?

> 
> > +
> > +	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)
> > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> > index 2661dda1a92b..64d4f1727da2 100644
> > --- a/net/bridge/br_private.h
> > +++ b/net/bridge/br_private.h
> > @@ -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 */
> > +	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?

ack, right. I agree.

> 
> > +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? :)

kfunc is probably too new :)

Regards,
Lorenzo

> 
> -Toke
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Toke Høiland-Jørgensen]

Lorenzo Bianconi <lorenzo@kernel.org> writes:

>> On Mon, Jan 24, 2022 at 10:32 AM Nikolay Aleksandrov <nikolay@nvidia.com> wrote:
>> > >
>> > > +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();
>> > > +
>> > > +     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'.
>
> ack, right. This is a very interesting approach. I will investigate
> it. Thanks.

I think it would be interesting to try both, and compare their
performance. I'm a bit sceptical about Alexei's assertion that
dev_get_by_index_rcu() is that expensive: we do such a lookup in the XDP
redirect code when using the non-map bpf_redirect() helper, and I have
not been able to measure a significant performance difference between
the map and non-map variants (after we added bulking to the latter).

If looking up devices by ifindex does turn out to be too expensive,
maybe what we really need is a way to pass around 'struct net_device'
pointers to BPF helpers, so a given BPF program only has to do the
lookup once if it's calling multiple dev-based helpers? I think this
should be doable with BTF, no?

-Toke

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Kumar Kartikeya Dwivedi]

On Wed, Jan 26, 2022 at 05:12:15PM IST, Lorenzo Bianconi wrote:
> > [ snip to focus on the API ]
> >
> > > +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

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.

>
>  [...]

--
Kartikeya

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Toke Høiland-Jørgensen]

Lorenzo Bianconi <lorenzo@kernel.org> writes:

>> > +	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?

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

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Kumar Kartikeya Dwivedi]

On Wed, Jan 26, 2022 at 04:57:42PM IST, Lorenzo Bianconi wrote:
> > On 24/01/2022 19:20, Lorenzo Bianconi wrote:
> > > 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 <lorenzo@kernel.org>
> > > ---
> > >  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,
>
> 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?
>

Yes, no need to call unregister (hence there is no unregister).

> > [...]



--
Kartikeya

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Nikolay Aleksandrov]

On 26/01/2022 13:27, Lorenzo Bianconi wrote:
>> On 24/01/2022 19:20, Lorenzo Bianconi wrote:
>>> 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 <lorenzo@kernel.org>
>>> ---
>>>  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,
> 
> Hi Nikolay,
> 
> thx for the review.
> 
>> 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.
> 
>>
>> 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?
> 

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.
One (untested, potential) way is to speedup full flows that have successfully passed from ingress to
egress for some period of time and flush them based on related events that might have affected
them, but that is very different. Another way would be to replicate some of that logic in ebpf
which would hit performance, and would probably also require more helpers. It would be interesting
to see how this problem would be solved.

>>
>>> diff --git a/net/bridge/br.c b/net/bridge/br.c
>>> index 1fac72cc617f..d2d1c2341d9c 100644
>>> --- a/net/bridge/br.c
>>> +++ b/net/bridge/br.c
>>> @@ -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
>>>  
>>> +#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?
> 

Oh, my bad. I obviously should've looked at the bpf tree. :)

>>
>>> +	}
>>> +#endif
>>> +
>>>  	return 0;
>>>  
>>>  err_out6:
>>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>>> index 6ccda68bd473..cd3afa240298 100644
>>> --- a/net/bridge/br_fdb.c
>>> +++ b/net/bridge/br_fdb.c
>>> @@ -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;
>>>  
>>>  	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.
> 
>>
>> 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) {
>  ...
>  }
> 
>>
>>> +		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.
> 
>>
>>> +	}
>>> +	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();
>>> +
>>> +	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
> 
>>
>>> +
>>> +	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)
>>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>>> index 2661dda1a92b..64d4f1727da2 100644
>>> --- a/net/bridge/br_private.h
>>> +++ b/net/bridge/br_private.h
>>> @@ -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 */
>>> +	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

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Toke Høiland-Jørgensen]

Nikolay Aleksandrov <nikolay@nvidia.com> writes:

> On 26/01/2022 13:27, Lorenzo Bianconi wrote:
>>> On 24/01/2022 19:20, Lorenzo Bianconi wrote:
>>>> 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 <lorenzo@kernel.org>
>>>> ---
>>>>  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,
>> 
>> Hi Nikolay,
>> 
>> thx for the review.
>> 
>>> 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.
>> 
>>>
>>> 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?
>> 
>
> 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

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Nikolay Aleksandrov]

On 26/01/2022 14:50, Toke Høiland-Jørgensen wrote:
> Nikolay Aleksandrov <nikolay@nvidia.com> writes:
> 
>> On 26/01/2022 13:27, Lorenzo Bianconi wrote:
>>>> On 24/01/2022 19:20, Lorenzo Bianconi wrote:
>>>>> 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 <lorenzo@kernel.org>
>>>>> ---
>>>>>  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,
>>>
>>> Hi Nikolay,
>>>
>>> thx for the review.
>>>
>>>> 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.
>>>
>>>>
>>>> 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?
>>>
>>
>> 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
> 

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

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

> Lorenzo Bianconi <lorenzo@kernel.org> writes:
> 
> >> > +	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?
> 
> 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")

ack, right, I missed it. I will fix it.

Regards,
Lorenzo

> 
> -Toke
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3963 bytes --]

> On 26/01/2022 14:50, Toke Høiland-Jørgensen wrote:
> > Nikolay Aleksandrov <nikolay@nvidia.com> writes:
> > 
> >> On 26/01/2022 13:27, Lorenzo Bianconi wrote:
> >>>> On 24/01/2022 19:20, Lorenzo Bianconi wrote:
> >>>>> 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 <lorenzo@kernel.org>
> >>>>> ---
> >>>>>  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,
> >>>
> >>> Hi Nikolay,
> >>>
> >>> thx for the review.
> >>>
> >>>> 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.
> >>>
> >>>>
> >>>> 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?
> >>>
> >>
> >> 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
> > 
> 
> 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.

ack, I agree but I need to review it first since I am not so familiar
with that codebase :)
Doing so we can compare this solution with the one proposed by Alexei.

Regards,
Lorenzo

> 
> Cheers,
>  Nik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC bpf-next 1/2] net: bridge: add unstable br_fdb_find_port_from_ifindex helper

[Andrii Nakryiko]

On Wed, Jan 26, 2022 at 4:05 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Wed, Jan 26, 2022 at 05:12:15PM IST, Lorenzo Bianconi wrote:
> > > [ snip to focus on the API ]
> > >
> > > > +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
>
> 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).

Without reading the rest of the thread, bpf_core_type_size(struct
bpf_fdb_lookup) would be a CO-RE-relocatable way to get the actual
size of the type in the kernel.

>
> 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.
>
> >
> >  [...]
>
> --
> Kartikeya