* [PATCH bpf-next v4 0/2] Optimize bpf_redirect_map()/xdp_do_redirect()
@ 2021-02-26 11:23 Björn Töpel
2021-02-26 11:23 ` [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation Björn Töpel
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Björn Töpel @ 2021-02-26 11:23 UTC (permalink / raw)
To: ast, daniel, netdev, bpf
Cc: Björn Töpel, bjorn.topel, maciej.fijalkowski, hawk,
toke, magnus.karlsson, john.fastabend, kuba, davem
Hi XDP-folks,
This two patch series contain two optimizations for the
bpf_redirect_map() helper and the xdp_do_redirect() function.
The bpf_redirect_map() optimization is about avoiding the map lookup
dispatching. Instead of having a switch-statement and selecting the
correct lookup function, we let bpf_redirect_map() be a map operation,
where each map has its own bpf_redirect_map() implementation. This way
the run-time lookup is avoided.
The xdp_do_redirect() patch restructures the code, so that the map
pointer indirection can be avoided.
Performance-wise I got 3% improvement for XSKMAP
(sample:xdpsock/rx-drop), and 4% (sample:xdp_redirect_map) on my
machine.
More details in each commit.
@Jesper/Toke I dropped your Acked-by: on the first patch, since there
were major restucturing. Please have another look! Thanks!
Changelog:
v3->v4: Made bpf_redirect_map() a map operation. (Daniel)
v2->v3: Fix build when CONFIG_NET is not set. (lkp)
v1->v2: Removed warning when CONFIG_BPF_SYSCALL was not set. (lkp)
Cleaned up case-clause in xdp_do_generic_redirect_map(). (Toke)
Re-added comment. (Toke)
rfc->v1: Use map_id, and remove bpf_clear_redirect_map(). (Toke)
Get rid of the macro and use __always_inline. (Jesper)
rfc: https://lore.kernel.org/bpf/87im7fy9nc.fsf@toke.dk/ (Cover not on lore!)
v1: https://lore.kernel.org/bpf/20210219145922.63655-1-bjorn.topel@gmail.com/
v2: https://lore.kernel.org/bpf/20210220153056.111968-1-bjorn.topel@gmail.com/
v3: https://lore.kernel.org/bpf/20210221200954.164125-3-bjorn.topel@gmail.com/
Cheers,
Björn
Björn Töpel (2):
bpf, xdp: make bpf_redirect_map() a map operation
bpf, xdp: restructure redirect actions
include/linux/bpf.h | 26 ++----
include/linux/filter.h | 39 +++++++-
include/net/xdp_sock.h | 19 ----
include/trace/events/xdp.h | 66 ++++++++-----
kernel/bpf/cpumap.c | 10 +-
kernel/bpf/devmap.c | 19 +++-
kernel/bpf/verifier.c | 11 ++-
net/core/filter.c | 183 ++++++++++++-------------------------
net/xdp/xskmap.c | 20 +++-
9 files changed, 195 insertions(+), 198 deletions(-)
base-commit: 9c8f21e6f8856a96634e542a58ef3abf27486801
--
2.27.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation
2021-02-26 11:23 [PATCH bpf-next v4 0/2] Optimize bpf_redirect_map()/xdp_do_redirect() Björn Töpel
@ 2021-02-26 11:23 ` Björn Töpel
2021-02-26 11:37 ` Toke Høiland-Jørgensen
` (2 more replies)
2021-02-26 11:23 ` [PATCH bpf-next v4 2/2] bpf, xdp: restructure redirect actions Björn Töpel
2021-02-26 11:35 ` [PATCH bpf-next v4 0/2] Optimize bpf_redirect_map()/xdp_do_redirect() Toke Høiland-Jørgensen
2 siblings, 3 replies; 17+ messages in thread
From: Björn Töpel @ 2021-02-26 11:23 UTC (permalink / raw)
To: ast, daniel, netdev, bpf
Cc: Björn Töpel, maciej.fijalkowski, hawk, toke,
magnus.karlsson, john.fastabend, kuba, davem
From: Björn Töpel <bjorn.topel@intel.com>
Currently the bpf_redirect_map() implementation dispatches to the
correct map-lookup function via a switch-statement. To avoid the
dispatching, this change adds bpf_redirect_map() as a map
operation. Each map provides its bpf_redirect_map() version, and
correct function is automatically selected by the BPF verifier.
A nice side-effect of the code movement is that the map lookup
functions are now local to the map implementation files, which removes
one additional function call.
Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
---
include/linux/bpf.h | 26 ++++++--------------------
include/linux/filter.h | 27 +++++++++++++++++++++++++++
include/net/xdp_sock.h | 19 -------------------
kernel/bpf/cpumap.c | 8 +++++++-
kernel/bpf/devmap.c | 16 ++++++++++++++--
kernel/bpf/verifier.c | 11 +++++++++--
net/core/filter.c | 39 +--------------------------------------
net/xdp/xskmap.c | 18 ++++++++++++++++++
8 files changed, 82 insertions(+), 82 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index cccaef1088ea..a44ba904ca37 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -117,6 +117,9 @@ struct bpf_map_ops {
void *owner, u32 size);
struct bpf_local_storage __rcu ** (*map_owner_storage_ptr)(void *owner);
+ /* XDP helpers.*/
+ int (*xdp_redirect_map)(struct bpf_map *map, u32 ifindex, u64 flags);
+
/* map_meta_equal must be implemented for maps that can be
* used as an inner map. It is a runtime check to ensure
* an inner map can be inserted to an outer map.
@@ -1429,9 +1432,9 @@ struct btf *bpf_get_btf_vmlinux(void);
/* Map specifics */
struct xdp_buff;
struct sk_buff;
+struct bpf_dtab_netdev;
+struct bpf_cpu_map_entry;
-struct bpf_dtab_netdev *__dev_map_lookup_elem(struct bpf_map *map, u32 key);
-struct bpf_dtab_netdev *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key);
void __dev_flush(void);
int dev_xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp,
struct net_device *dev_rx);
@@ -1441,7 +1444,6 @@ int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, struct sk_buff *skb,
struct bpf_prog *xdp_prog);
bool dev_map_can_have_prog(struct bpf_map *map);
-struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key);
void __cpu_map_flush(void);
int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_buff *xdp,
struct net_device *dev_rx);
@@ -1568,17 +1570,6 @@ static inline int bpf_obj_get_user(const char __user *pathname, int flags)
return -EOPNOTSUPP;
}
-static inline struct net_device *__dev_map_lookup_elem(struct bpf_map *map,
- u32 key)
-{
- return NULL;
-}
-
-static inline struct net_device *__dev_map_hash_lookup_elem(struct bpf_map *map,
- u32 key)
-{
- return NULL;
-}
static inline bool dev_map_can_have_prog(struct bpf_map *map)
{
return false;
@@ -1590,6 +1581,7 @@ static inline void __dev_flush(void)
struct xdp_buff;
struct bpf_dtab_netdev;
+struct bpf_cpu_map_entry;
static inline
int dev_xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp,
@@ -1614,12 +1606,6 @@ static inline int dev_map_generic_redirect(struct bpf_dtab_netdev *dst,
return 0;
}
-static inline
-struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key)
-{
- return NULL;
-}
-
static inline void __cpu_map_flush(void)
{
}
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 3b00fc906ccd..008691fd3b58 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1472,4 +1472,31 @@ static inline bool bpf_sk_lookup_run_v6(struct net *net, int protocol,
}
#endif /* IS_ENABLED(CONFIG_IPV6) */
+static __always_inline int __bpf_xdp_redirect_map(struct bpf_map *map, u32 ifindex, u64 flags,
+ void *lookup_elem(struct bpf_map *map, u32 key))
+{
+ struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
+
+ /* Lower bits of the flags are used as return code on lookup failure */
+ if (unlikely(flags > XDP_TX))
+ return XDP_ABORTED;
+
+ ri->tgt_value = lookup_elem(map, ifindex);
+ if (unlikely(!ri->tgt_value)) {
+ /* If the lookup fails we want to clear out the state in the
+ * redirect_info struct completely, so that if an eBPF program
+ * performs multiple lookups, the last one always takes
+ * precedence.
+ */
+ WRITE_ONCE(ri->map, NULL);
+ return flags;
+ }
+
+ ri->flags = flags;
+ ri->tgt_index = ifindex;
+ WRITE_ONCE(ri->map, map);
+
+ return XDP_REDIRECT;
+}
+
#endif /* __LINUX_FILTER_H__ */
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index cc17bc957548..9c0722c6d7ac 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -80,19 +80,6 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp);
int __xsk_map_redirect(struct xdp_sock *xs, struct xdp_buff *xdp);
void __xsk_map_flush(void);
-static inline struct xdp_sock *__xsk_map_lookup_elem(struct bpf_map *map,
- u32 key)
-{
- struct xsk_map *m = container_of(map, struct xsk_map, map);
- struct xdp_sock *xs;
-
- if (key >= map->max_entries)
- return NULL;
-
- xs = READ_ONCE(m->xsk_map[key]);
- return xs;
-}
-
#else
static inline int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
@@ -109,12 +96,6 @@ static inline void __xsk_map_flush(void)
{
}
-static inline struct xdp_sock *__xsk_map_lookup_elem(struct bpf_map *map,
- u32 key)
-{
- return NULL;
-}
-
#endif /* CONFIG_XDP_SOCKETS */
#endif /* _LINUX_XDP_SOCK_H */
diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index 5d1469de6921..85a2d33fd46b 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -563,7 +563,7 @@ static void cpu_map_free(struct bpf_map *map)
kfree(cmap);
}
-struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key)
+static void *__cpu_map_lookup_elem(struct bpf_map *map, u32 key)
{
struct bpf_cpu_map *cmap = container_of(map, struct bpf_cpu_map, map);
struct bpf_cpu_map_entry *rcpu;
@@ -600,6 +600,11 @@ static int cpu_map_get_next_key(struct bpf_map *map, void *key, void *next_key)
return 0;
}
+static int cpu_map_xdp_redirect_map(struct bpf_map *map, u32 ifindex, u64 flags)
+{
+ return __bpf_xdp_redirect_map(map, ifindex, flags, __cpu_map_lookup_elem);
+}
+
static int cpu_map_btf_id;
const struct bpf_map_ops cpu_map_ops = {
.map_meta_equal = bpf_map_meta_equal,
@@ -612,6 +617,7 @@ const struct bpf_map_ops cpu_map_ops = {
.map_check_btf = map_check_no_btf,
.map_btf_name = "bpf_cpu_map",
.map_btf_id = &cpu_map_btf_id,
+ .xdp_redirect_map = cpu_map_xdp_redirect_map,
};
static void bq_flush_to_queue(struct xdp_bulk_queue *bq)
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index 85d9d1b72a33..adf9a2517f80 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -258,7 +258,7 @@ static int dev_map_get_next_key(struct bpf_map *map, void *key, void *next_key)
return 0;
}
-struct bpf_dtab_netdev *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key)
+static void *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key)
{
struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
struct hlist_head *head = dev_map_index_hash(dtab, key);
@@ -392,7 +392,7 @@ void __dev_flush(void)
* update happens in parallel here a dev_put wont happen until after reading the
* ifindex.
*/
-struct bpf_dtab_netdev *__dev_map_lookup_elem(struct bpf_map *map, u32 key)
+static void *__dev_map_lookup_elem(struct bpf_map *map, u32 key)
{
struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
struct bpf_dtab_netdev *obj;
@@ -735,6 +735,16 @@ static int dev_map_hash_update_elem(struct bpf_map *map, void *key, void *value,
map, key, value, map_flags);
}
+static int dev_map_xdp_redirect_map(struct bpf_map *map, u32 ifindex, u64 flags)
+{
+ return __bpf_xdp_redirect_map(map, ifindex, flags, __dev_map_lookup_elem);
+}
+
+static int dev_hash_map_xdp_redirect_map(struct bpf_map *map, u32 ifindex, u64 flags)
+{
+ return __bpf_xdp_redirect_map(map, ifindex, flags, __dev_map_hash_lookup_elem);
+}
+
static int dev_map_btf_id;
const struct bpf_map_ops dev_map_ops = {
.map_meta_equal = bpf_map_meta_equal,
@@ -747,6 +757,7 @@ const struct bpf_map_ops dev_map_ops = {
.map_check_btf = map_check_no_btf,
.map_btf_name = "bpf_dtab",
.map_btf_id = &dev_map_btf_id,
+ .xdp_redirect_map = dev_map_xdp_redirect_map,
};
static int dev_map_hash_map_btf_id;
@@ -761,6 +772,7 @@ const struct bpf_map_ops dev_map_hash_ops = {
.map_check_btf = map_check_no_btf,
.map_btf_name = "bpf_dtab",
.map_btf_id = &dev_map_hash_map_btf_id,
+ .xdp_redirect_map = dev_hash_map_xdp_redirect_map,
};
static void dev_map_hash_remove_netdev(struct bpf_dtab *dtab,
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1dda9d81f12c..96705a49225e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5409,7 +5409,8 @@ record_func_map(struct bpf_verifier_env *env, struct bpf_call_arg_meta *meta,
func_id != BPF_FUNC_map_delete_elem &&
func_id != BPF_FUNC_map_push_elem &&
func_id != BPF_FUNC_map_pop_elem &&
- func_id != BPF_FUNC_map_peek_elem)
+ func_id != BPF_FUNC_map_peek_elem &&
+ func_id != BPF_FUNC_redirect_map)
return 0;
if (map == NULL) {
@@ -11762,7 +11763,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
insn->imm == BPF_FUNC_map_delete_elem ||
insn->imm == BPF_FUNC_map_push_elem ||
insn->imm == BPF_FUNC_map_pop_elem ||
- insn->imm == BPF_FUNC_map_peek_elem)) {
+ insn->imm == BPF_FUNC_map_peek_elem ||
+ insn->imm == BPF_FUNC_redirect_map)) {
aux = &env->insn_aux_data[i + delta];
if (bpf_map_ptr_poisoned(aux))
goto patch_call_imm;
@@ -11804,6 +11806,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
(int (*)(struct bpf_map *map, void *value))NULL));
BUILD_BUG_ON(!__same_type(ops->map_peek_elem,
(int (*)(struct bpf_map *map, void *value))NULL));
+ BUILD_BUG_ON(!__same_type(ops->xdp_redirect_map,
+ (int (*)(struct bpf_map *map, u32 ifindex, u64 flags))NULL));
patch_map_ops_generic:
switch (insn->imm) {
case BPF_FUNC_map_lookup_elem:
@@ -11830,6 +11834,9 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
insn->imm = BPF_CAST_CALL(ops->map_peek_elem) -
__bpf_call_base;
continue;
+ case BPF_FUNC_redirect_map:
+ insn->imm = BPF_CAST_CALL(ops->xdp_redirect_map) - __bpf_call_base;
+ continue;
}
goto patch_call_imm;
diff --git a/net/core/filter.c b/net/core/filter.c
index adfdad234674..fdf7401f43fd 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3944,22 +3944,6 @@ void xdp_do_flush(void)
}
EXPORT_SYMBOL_GPL(xdp_do_flush);
-static inline void *__xdp_map_lookup_elem(struct bpf_map *map, u32 index)
-{
- switch (map->map_type) {
- case BPF_MAP_TYPE_DEVMAP:
- return __dev_map_lookup_elem(map, index);
- case BPF_MAP_TYPE_DEVMAP_HASH:
- return __dev_map_hash_lookup_elem(map, index);
- case BPF_MAP_TYPE_CPUMAP:
- return __cpu_map_lookup_elem(map, index);
- case BPF_MAP_TYPE_XSKMAP:
- return __xsk_map_lookup_elem(map, index);
- default:
- return NULL;
- }
-}
-
void bpf_clear_redirect_map(struct bpf_map *map)
{
struct bpf_redirect_info *ri;
@@ -4113,28 +4097,7 @@ static const struct bpf_func_proto bpf_xdp_redirect_proto = {
BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex,
u64, flags)
{
- struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
-
- /* Lower bits of the flags are used as return code on lookup failure */
- if (unlikely(flags > XDP_TX))
- return XDP_ABORTED;
-
- ri->tgt_value = __xdp_map_lookup_elem(map, ifindex);
- if (unlikely(!ri->tgt_value)) {
- /* If the lookup fails we want to clear out the state in the
- * redirect_info struct completely, so that if an eBPF program
- * performs multiple lookups, the last one always takes
- * precedence.
- */
- WRITE_ONCE(ri->map, NULL);
- return flags;
- }
-
- ri->flags = flags;
- ri->tgt_index = ifindex;
- WRITE_ONCE(ri->map, map);
-
- return XDP_REDIRECT;
+ return map->ops->xdp_redirect_map(map, ifindex, flags);
}
static const struct bpf_func_proto bpf_xdp_redirect_map_proto = {
diff --git a/net/xdp/xskmap.c b/net/xdp/xskmap.c
index 113fd9017203..92f4023d3ae2 100644
--- a/net/xdp/xskmap.c
+++ b/net/xdp/xskmap.c
@@ -125,6 +125,18 @@ static int xsk_map_gen_lookup(struct bpf_map *map, struct bpf_insn *insn_buf)
return insn - insn_buf;
}
+static void *__xsk_map_lookup_elem(struct bpf_map *map, u32 key)
+{
+ struct xsk_map *m = container_of(map, struct xsk_map, map);
+ struct xdp_sock *xs;
+
+ if (key >= map->max_entries)
+ return NULL;
+
+ xs = READ_ONCE(m->xsk_map[key]);
+ return xs;
+}
+
static void *xsk_map_lookup_elem(struct bpf_map *map, void *key)
{
WARN_ON_ONCE(!rcu_read_lock_held());
@@ -215,6 +227,11 @@ static int xsk_map_delete_elem(struct bpf_map *map, void *key)
return 0;
}
+static int xsk_map_xdp_redirect_map(struct bpf_map *map, u32 ifindex, u64 flags)
+{
+ return __bpf_xdp_redirect_map(map, ifindex, flags, __xsk_map_lookup_elem);
+}
+
void xsk_map_try_sock_delete(struct xsk_map *map, struct xdp_sock *xs,
struct xdp_sock **map_entry)
{
@@ -247,4 +264,5 @@ const struct bpf_map_ops xsk_map_ops = {
.map_check_btf = map_check_no_btf,
.map_btf_name = "xsk_map",
.map_btf_id = &xsk_map_btf_id,
+ .xdp_redirect_map = xsk_map_xdp_redirect_map,
};
--
2.27.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH bpf-next v4 2/2] bpf, xdp: restructure redirect actions
2021-02-26 11:23 [PATCH bpf-next v4 0/2] Optimize bpf_redirect_map()/xdp_do_redirect() Björn Töpel
2021-02-26 11:23 ` [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation Björn Töpel
@ 2021-02-26 11:23 ` Björn Töpel
2021-02-26 11:35 ` [PATCH bpf-next v4 0/2] Optimize bpf_redirect_map()/xdp_do_redirect() Toke Høiland-Jørgensen
2 siblings, 0 replies; 17+ messages in thread
From: Björn Töpel @ 2021-02-26 11:23 UTC (permalink / raw)
To: ast, daniel, netdev, bpf
Cc: Björn Töpel, maciej.fijalkowski, hawk, toke,
magnus.karlsson, john.fastabend, kuba, davem,
Jesper Dangaard Brouer
From: Björn Töpel <bjorn.topel@intel.com>
The XDP_REDIRECT implementations for maps and non-maps are fairly
similar, but obviously need to take different code paths depending on
if the target is using a map or not. Today, the redirect targets for
XDP either uses a map, or is based on ifindex.
Here, an explicit redirect type is added to bpf_redirect_info, instead
of the actual map. Redirect type, map item/ifindex, and the map_id (if
any) is passed to xdp_do_redirect().
In addition to making the code easier to follow, using an explicit
type in bpf_redirect_info has a slight positive performance impact by
avoiding a pointer indirection for the map type lookup, and instead
use the cacheline for bpf_redirect_info.
Since the actual map is not passed via bpf_redirect_info anymore, the
map lookup is only done in the BPF helper. This means that the
bpf_clear_redirect_map() function can be removed. The actual map item
is RCU protected.
The bpf_redirect_info flags member is not used by XDP, and not
read/written any more. The map member is only written to when
required/used, and not unconditionally.
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
---
include/linux/filter.h | 20 ++++--
include/trace/events/xdp.h | 66 ++++++++++-------
kernel/bpf/cpumap.c | 4 +-
kernel/bpf/devmap.c | 7 +-
net/core/filter.c | 144 +++++++++++++++----------------------
net/xdp/xskmap.c | 4 +-
6 files changed, 121 insertions(+), 124 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 008691fd3b58..a7752badc2ec 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -646,11 +646,20 @@ struct bpf_redirect_info {
u32 flags;
u32 tgt_index;
void *tgt_value;
- struct bpf_map *map;
+ u32 map_id;
+ u32 tgt_type;
u32 kern_flags;
struct bpf_nh_params nh;
};
+enum xdp_redirect_type {
+ XDP_REDIR_UNSET,
+ XDP_REDIR_DEV_IFINDEX,
+ XDP_REDIR_DEV_MAP,
+ XDP_REDIR_CPU_MAP,
+ XDP_REDIR_XSK_MAP,
+};
+
DECLARE_PER_CPU(struct bpf_redirect_info, bpf_redirect_info);
/* flags for bpf_redirect_info kern_flags */
@@ -1473,7 +1482,8 @@ static inline bool bpf_sk_lookup_run_v6(struct net *net, int protocol,
#endif /* IS_ENABLED(CONFIG_IPV6) */
static __always_inline int __bpf_xdp_redirect_map(struct bpf_map *map, u32 ifindex, u64 flags,
- void *lookup_elem(struct bpf_map *map, u32 key))
+ void *lookup_elem(struct bpf_map *map, u32 key),
+ enum xdp_redirect_type type)
{
struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
@@ -1488,13 +1498,13 @@ static __always_inline int __bpf_xdp_redirect_map(struct bpf_map *map, u32 ifind
* performs multiple lookups, the last one always takes
* precedence.
*/
- WRITE_ONCE(ri->map, NULL);
+ ri->tgt_type = XDP_REDIR_UNSET;
return flags;
}
- ri->flags = flags;
ri->tgt_index = ifindex;
- WRITE_ONCE(ri->map, map);
+ ri->tgt_type = type;
+ ri->map_id = map->id;
return XDP_REDIRECT;
}
diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index 76a97176ab81..538321735447 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h
@@ -86,19 +86,15 @@ struct _bpf_dtab_netdev {
};
#endif /* __DEVMAP_OBJ_TYPE */
-#define devmap_ifindex(tgt, map) \
- (((map->map_type == BPF_MAP_TYPE_DEVMAP || \
- map->map_type == BPF_MAP_TYPE_DEVMAP_HASH)) ? \
- ((struct _bpf_dtab_netdev *)tgt)->dev->ifindex : 0)
-
DECLARE_EVENT_CLASS(xdp_redirect_template,
TP_PROTO(const struct net_device *dev,
const struct bpf_prog *xdp,
const void *tgt, int err,
- const struct bpf_map *map, u32 index),
+ enum xdp_redirect_type type,
+ const struct bpf_redirect_info *ri),
- TP_ARGS(dev, xdp, tgt, err, map, index),
+ TP_ARGS(dev, xdp, tgt, err, type, ri),
TP_STRUCT__entry(
__field(int, prog_id)
@@ -111,14 +107,30 @@ DECLARE_EVENT_CLASS(xdp_redirect_template,
),
TP_fast_assign(
+ u32 ifindex = 0, map_id = 0, index = ri->tgt_index;
+
+ switch (type) {
+ case XDP_REDIR_DEV_MAP:
+ ifindex = ((struct _bpf_dtab_netdev *)tgt)->dev->ifindex;
+ fallthrough;
+ case XDP_REDIR_CPU_MAP:
+ case XDP_REDIR_XSK_MAP:
+ map_id = ri->map_id;
+ break;
+ case XDP_REDIR_DEV_IFINDEX:
+ ifindex = (u32)(long)tgt;
+ break;
+ default:
+ break;
+ }
+
__entry->prog_id = xdp->aux->id;
__entry->act = XDP_REDIRECT;
__entry->ifindex = dev->ifindex;
__entry->err = err;
- __entry->to_ifindex = map ? devmap_ifindex(tgt, map) :
- index;
- __entry->map_id = map ? map->id : 0;
- __entry->map_index = map ? index : 0;
+ __entry->to_ifindex = ifindex;
+ __entry->map_id = map_id;
+ __entry->map_index = index;
),
TP_printk("prog_id=%d action=%s ifindex=%d to_ifindex=%d err=%d"
@@ -133,45 +145,49 @@ DEFINE_EVENT(xdp_redirect_template, xdp_redirect,
TP_PROTO(const struct net_device *dev,
const struct bpf_prog *xdp,
const void *tgt, int err,
- const struct bpf_map *map, u32 index),
- TP_ARGS(dev, xdp, tgt, err, map, index)
+ enum xdp_redirect_type type,
+ const struct bpf_redirect_info *ri),
+ TP_ARGS(dev, xdp, tgt, err, type, ri)
);
DEFINE_EVENT(xdp_redirect_template, xdp_redirect_err,
TP_PROTO(const struct net_device *dev,
const struct bpf_prog *xdp,
const void *tgt, int err,
- const struct bpf_map *map, u32 index),
- TP_ARGS(dev, xdp, tgt, err, map, index)
+ enum xdp_redirect_type type,
+ const struct bpf_redirect_info *ri),
+ TP_ARGS(dev, xdp, tgt, err, type, ri)
);
#define _trace_xdp_redirect(dev, xdp, to) \
- trace_xdp_redirect(dev, xdp, NULL, 0, NULL, to)
+ trace_xdp_redirect(dev, xdp, NULL, 0, XDP_REDIR_DEV_IFINDEX, NULL)
#define _trace_xdp_redirect_err(dev, xdp, to, err) \
- trace_xdp_redirect_err(dev, xdp, NULL, err, NULL, to)
+ trace_xdp_redirect_err(dev, xdp, NULL, err, XDP_REDIR_DEV_IFINDEX, NULL)
-#define _trace_xdp_redirect_map(dev, xdp, to, map, index) \
- trace_xdp_redirect(dev, xdp, to, 0, map, index)
+#define _trace_xdp_redirect_map(dev, xdp, to, type, ri) \
+ trace_xdp_redirect(dev, xdp, to, 0, type, ri)
-#define _trace_xdp_redirect_map_err(dev, xdp, to, map, index, err) \
- trace_xdp_redirect_err(dev, xdp, to, err, map, index)
+#define _trace_xdp_redirect_map_err(dev, xdp, to, type, ri, err) \
+ trace_xdp_redirect_err(dev, xdp, to, err, type, ri)
/* not used anymore, but kept around so as not to break old programs */
DEFINE_EVENT(xdp_redirect_template, xdp_redirect_map,
TP_PROTO(const struct net_device *dev,
const struct bpf_prog *xdp,
const void *tgt, int err,
- const struct bpf_map *map, u32 index),
- TP_ARGS(dev, xdp, tgt, err, map, index)
+ enum xdp_redirect_type type,
+ const struct bpf_redirect_info *ri),
+ TP_ARGS(dev, xdp, tgt, err, type, ri)
);
DEFINE_EVENT(xdp_redirect_template, xdp_redirect_map_err,
TP_PROTO(const struct net_device *dev,
const struct bpf_prog *xdp,
const void *tgt, int err,
- const struct bpf_map *map, u32 index),
- TP_ARGS(dev, xdp, tgt, err, map, index)
+ enum xdp_redirect_type type,
+ const struct bpf_redirect_info *ri),
+ TP_ARGS(dev, xdp, tgt, err, type, ri)
);
TRACE_EVENT(xdp_cpumap_kthread,
diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index 85a2d33fd46b..5161ccb871e7 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -543,7 +543,6 @@ static void cpu_map_free(struct bpf_map *map)
* complete.
*/
- bpf_clear_redirect_map(map);
synchronize_rcu();
/* For cpu_map the remote CPUs can still be using the entries
@@ -602,7 +601,8 @@ static int cpu_map_get_next_key(struct bpf_map *map, void *key, void *next_key)
static int cpu_map_xdp_redirect_map(struct bpf_map *map, u32 ifindex, u64 flags)
{
- return __bpf_xdp_redirect_map(map, ifindex, flags, __cpu_map_lookup_elem);
+ return __bpf_xdp_redirect_map(map, ifindex, flags, __cpu_map_lookup_elem,
+ XDP_REDIR_CPU_MAP);
}
static int cpu_map_btf_id;
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index adf9a2517f80..5a403471065b 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -197,7 +197,6 @@ static void dev_map_free(struct bpf_map *map)
list_del_rcu(&dtab->list);
spin_unlock(&dev_map_lock);
- bpf_clear_redirect_map(map);
synchronize_rcu();
/* Make sure prior __dev_map_entry_free() have completed. */
@@ -737,12 +736,14 @@ static int dev_map_hash_update_elem(struct bpf_map *map, void *key, void *value,
static int dev_map_xdp_redirect_map(struct bpf_map *map, u32 ifindex, u64 flags)
{
- return __bpf_xdp_redirect_map(map, ifindex, flags, __dev_map_lookup_elem);
+ return __bpf_xdp_redirect_map(map, ifindex, flags, __dev_map_lookup_elem,
+ XDP_REDIR_DEV_MAP);
}
static int dev_hash_map_xdp_redirect_map(struct bpf_map *map, u32 ifindex, u64 flags)
{
- return __bpf_xdp_redirect_map(map, ifindex, flags, __dev_map_hash_lookup_elem);
+ return __bpf_xdp_redirect_map(map, ifindex, flags, __dev_map_hash_lookup_elem,
+ XDP_REDIR_DEV_MAP);
}
static int dev_map_btf_id;
diff --git a/net/core/filter.c b/net/core/filter.c
index fdf7401f43fd..8ce21667b899 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3919,23 +3919,6 @@ static const struct bpf_func_proto bpf_xdp_adjust_meta_proto = {
.arg2_type = ARG_ANYTHING,
};
-static int __bpf_tx_xdp_map(struct net_device *dev_rx, void *fwd,
- struct bpf_map *map, struct xdp_buff *xdp)
-{
- switch (map->map_type) {
- case BPF_MAP_TYPE_DEVMAP:
- case BPF_MAP_TYPE_DEVMAP_HASH:
- return dev_map_enqueue(fwd, xdp, dev_rx);
- case BPF_MAP_TYPE_CPUMAP:
- return cpu_map_enqueue(fwd, xdp, dev_rx);
- case BPF_MAP_TYPE_XSKMAP:
- return __xsk_map_redirect(fwd, xdp);
- default:
- return -EBADRQC;
- }
- return 0;
-}
-
void xdp_do_flush(void)
{
__dev_flush();
@@ -3944,55 +3927,45 @@ void xdp_do_flush(void)
}
EXPORT_SYMBOL_GPL(xdp_do_flush);
-void bpf_clear_redirect_map(struct bpf_map *map)
-{
- struct bpf_redirect_info *ri;
- int cpu;
-
- for_each_possible_cpu(cpu) {
- ri = per_cpu_ptr(&bpf_redirect_info, cpu);
- /* Avoid polluting remote cacheline due to writes if
- * not needed. Once we pass this test, we need the
- * cmpxchg() to make sure it hasn't been changed in
- * the meantime by remote CPU.
- */
- if (unlikely(READ_ONCE(ri->map) == map))
- cmpxchg(&ri->map, map, NULL);
- }
-}
-
int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
struct bpf_prog *xdp_prog)
{
struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
- struct bpf_map *map = READ_ONCE(ri->map);
- u32 index = ri->tgt_index;
+ enum xdp_redirect_type type = ri->tgt_type;
void *fwd = ri->tgt_value;
int err;
- ri->tgt_index = 0;
- ri->tgt_value = NULL;
- WRITE_ONCE(ri->map, NULL);
+ ri->tgt_type = XDP_REDIR_UNSET;
- if (unlikely(!map)) {
- fwd = dev_get_by_index_rcu(dev_net(dev), index);
+ switch (type) {
+ case XDP_REDIR_DEV_IFINDEX:
+ fwd = dev_get_by_index_rcu(dev_net(dev), (u32)(long)fwd);
if (unlikely(!fwd)) {
err = -EINVAL;
- goto err;
+ break;
}
-
err = dev_xdp_enqueue(fwd, xdp, dev);
- } else {
- err = __bpf_tx_xdp_map(dev, fwd, map, xdp);
+ break;
+ case XDP_REDIR_DEV_MAP:
+ err = dev_map_enqueue(fwd, xdp, dev);
+ break;
+ case XDP_REDIR_CPU_MAP:
+ err = cpu_map_enqueue(fwd, xdp, dev);
+ break;
+ case XDP_REDIR_XSK_MAP:
+ err = __xsk_map_redirect(fwd, xdp);
+ break;
+ default:
+ err = -EBADRQC;
}
if (unlikely(err))
goto err;
- _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
+ _trace_xdp_redirect_map(dev, xdp_prog, fwd, type, ri);
return 0;
err:
- _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
+ _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, type, ri, err);
return err;
}
EXPORT_SYMBOL_GPL(xdp_do_redirect);
@@ -4001,41 +3974,37 @@ static int xdp_do_generic_redirect_map(struct net_device *dev,
struct sk_buff *skb,
struct xdp_buff *xdp,
struct bpf_prog *xdp_prog,
- struct bpf_map *map)
+ void *fwd,
+ enum xdp_redirect_type type)
{
struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
- u32 index = ri->tgt_index;
- void *fwd = ri->tgt_value;
- int err = 0;
-
- ri->tgt_index = 0;
- ri->tgt_value = NULL;
- WRITE_ONCE(ri->map, NULL);
-
- if (map->map_type == BPF_MAP_TYPE_DEVMAP ||
- map->map_type == BPF_MAP_TYPE_DEVMAP_HASH) {
- struct bpf_dtab_netdev *dst = fwd;
+ int err;
- err = dev_map_generic_redirect(dst, skb, xdp_prog);
+ switch (type) {
+ case XDP_REDIR_DEV_MAP:
+ err = dev_map_generic_redirect(fwd, skb, xdp_prog);
if (unlikely(err))
goto err;
- } else if (map->map_type == BPF_MAP_TYPE_XSKMAP) {
+ break;
+ case XDP_REDIR_XSK_MAP: {
struct xdp_sock *xs = fwd;
err = xsk_generic_rcv(xs, xdp);
if (err)
goto err;
consume_skb(skb);
- } else {
+ break;
+ }
+ default:
/* TODO: Handle BPF_MAP_TYPE_CPUMAP */
err = -EBADRQC;
goto err;
}
- _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index);
+ _trace_xdp_redirect_map(dev, xdp_prog, fwd, type, ri);
return 0;
err:
- _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err);
+ _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, type, ri, err);
return err;
}
@@ -4043,29 +4012,31 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
struct xdp_buff *xdp, struct bpf_prog *xdp_prog)
{
struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
- struct bpf_map *map = READ_ONCE(ri->map);
- u32 index = ri->tgt_index;
- struct net_device *fwd;
+ enum xdp_redirect_type type = ri->tgt_type;
+ void *fwd = ri->tgt_value;
int err = 0;
- if (map)
- return xdp_do_generic_redirect_map(dev, skb, xdp, xdp_prog,
- map);
- ri->tgt_index = 0;
- fwd = dev_get_by_index_rcu(dev_net(dev), index);
- if (unlikely(!fwd)) {
- err = -EINVAL;
- goto err;
- }
+ ri->tgt_type = XDP_REDIR_UNSET;
+ ri->tgt_value = NULL;
- err = xdp_ok_fwd_dev(fwd, skb->len);
- if (unlikely(err))
- goto err;
+ if (type == XDP_REDIR_DEV_IFINDEX) {
+ fwd = dev_get_by_index_rcu(dev_net(dev), (u32)(long)fwd);
+ if (unlikely(!fwd)) {
+ err = -EINVAL;
+ goto err;
+ }
- skb->dev = fwd;
- _trace_xdp_redirect(dev, xdp_prog, index);
- generic_xdp_tx(skb, xdp_prog);
- return 0;
+ err = xdp_ok_fwd_dev(fwd, skb->len);
+ if (unlikely(err))
+ goto err;
+
+ skb->dev = fwd;
+ _trace_xdp_redirect(dev, xdp_prog, index);
+ generic_xdp_tx(skb, xdp_prog);
+ return 0;
+ }
+
+ return xdp_do_generic_redirect_map(dev, skb, xdp, xdp_prog, fwd, type);
err:
_trace_xdp_redirect_err(dev, xdp_prog, index, err);
return err;
@@ -4078,10 +4049,9 @@ BPF_CALL_2(bpf_xdp_redirect, u32, ifindex, u64, flags)
if (unlikely(flags))
return XDP_ABORTED;
- ri->flags = flags;
- ri->tgt_index = ifindex;
- ri->tgt_value = NULL;
- WRITE_ONCE(ri->map, NULL);
+ ri->tgt_type = XDP_REDIR_DEV_IFINDEX;
+ ri->tgt_index = 0;
+ ri->tgt_value = (void *)(long)ifindex;
return XDP_REDIRECT;
}
diff --git a/net/xdp/xskmap.c b/net/xdp/xskmap.c
index 92f4023d3ae2..d3e80c9fa17c 100644
--- a/net/xdp/xskmap.c
+++ b/net/xdp/xskmap.c
@@ -87,7 +87,6 @@ static void xsk_map_free(struct bpf_map *map)
{
struct xsk_map *m = container_of(map, struct xsk_map, map);
- bpf_clear_redirect_map(map);
synchronize_net();
bpf_map_area_free(m);
}
@@ -229,7 +228,8 @@ static int xsk_map_delete_elem(struct bpf_map *map, void *key)
static int xsk_map_xdp_redirect_map(struct bpf_map *map, u32 ifindex, u64 flags)
{
- return __bpf_xdp_redirect_map(map, ifindex, flags, __xsk_map_lookup_elem);
+ return __bpf_xdp_redirect_map(map, ifindex, flags, __xsk_map_lookup_elem,
+ XDP_REDIR_XSK_MAP);
}
void xsk_map_try_sock_delete(struct xsk_map *map, struct xdp_sock *xs,
--
2.27.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 0/2] Optimize bpf_redirect_map()/xdp_do_redirect()
2021-02-26 11:23 [PATCH bpf-next v4 0/2] Optimize bpf_redirect_map()/xdp_do_redirect() Björn Töpel
2021-02-26 11:23 ` [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation Björn Töpel
2021-02-26 11:23 ` [PATCH bpf-next v4 2/2] bpf, xdp: restructure redirect actions Björn Töpel
@ 2021-02-26 11:35 ` Toke Høiland-Jørgensen
2021-02-26 11:38 ` Björn Töpel
2 siblings, 1 reply; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-02-26 11:35 UTC (permalink / raw)
To: Björn Töpel, ast, daniel, netdev, bpf
Cc: Björn Töpel, bjorn.topel, maciej.fijalkowski, hawk,
magnus.karlsson, john.fastabend, kuba, davem
Björn Töpel <bjorn.topel@gmail.com> writes:
> Hi XDP-folks,
>
> This two patch series contain two optimizations for the
> bpf_redirect_map() helper and the xdp_do_redirect() function.
>
> The bpf_redirect_map() optimization is about avoiding the map lookup
> dispatching. Instead of having a switch-statement and selecting the
> correct lookup function, we let bpf_redirect_map() be a map operation,
> where each map has its own bpf_redirect_map() implementation. This way
> the run-time lookup is avoided.
>
> The xdp_do_redirect() patch restructures the code, so that the map
> pointer indirection can be avoided.
>
> Performance-wise I got 3% improvement for XSKMAP
> (sample:xdpsock/rx-drop), and 4% (sample:xdp_redirect_map) on my
> machine.
>
> More details in each commit.
>
> @Jesper/Toke I dropped your Acked-by: on the first patch, since there
> were major restucturing. Please have another look! Thanks!
Will do! Did you update the performance numbers above after that change?
-Toke
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation
2021-02-26 11:23 ` [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation Björn Töpel
@ 2021-02-26 11:37 ` Toke Høiland-Jørgensen
2021-02-26 11:40 ` Björn Töpel
2021-02-26 14:29 ` Jesper Dangaard Brouer
2021-02-26 15:23 ` kernel test robot
2021-02-26 21:48 ` Daniel Borkmann
2 siblings, 2 replies; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-02-26 11:37 UTC (permalink / raw)
To: Björn Töpel, ast, daniel, netdev, bpf
Cc: Björn Töpel, maciej.fijalkowski, hawk, magnus.karlsson,
john.fastabend, kuba, davem
Björn Töpel <bjorn.topel@gmail.com> writes:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> Currently the bpf_redirect_map() implementation dispatches to the
> correct map-lookup function via a switch-statement. To avoid the
> dispatching, this change adds bpf_redirect_map() as a map
> operation. Each map provides its bpf_redirect_map() version, and
> correct function is automatically selected by the BPF verifier.
>
> A nice side-effect of the code movement is that the map lookup
> functions are now local to the map implementation files, which removes
> one additional function call.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
Nice! I agree that this is a much nicer approach! :)
(That last paragraph above is why I asked if you updated the performance
numbers in the cover letter; removing an additional function call should
affect those, right?)
Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 0/2] Optimize bpf_redirect_map()/xdp_do_redirect()
2021-02-26 11:35 ` [PATCH bpf-next v4 0/2] Optimize bpf_redirect_map()/xdp_do_redirect() Toke Høiland-Jørgensen
@ 2021-02-26 11:38 ` Björn Töpel
2021-02-26 11:43 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 17+ messages in thread
From: Björn Töpel @ 2021-02-26 11:38 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, Björn Töpel, ast,
daniel, netdev, bpf
Cc: maciej.fijalkowski, hawk, magnus.karlsson, john.fastabend, kuba, davem
On 2021-02-26 12:35, Toke Høiland-Jørgensen wrote:
> Björn Töpel <bjorn.topel@gmail.com> writes:
>
>> Hi XDP-folks,
>>
>> This two patch series contain two optimizations for the
>> bpf_redirect_map() helper and the xdp_do_redirect() function.
>>
>> The bpf_redirect_map() optimization is about avoiding the map lookup
>> dispatching. Instead of having a switch-statement and selecting the
>> correct lookup function, we let bpf_redirect_map() be a map operation,
>> where each map has its own bpf_redirect_map() implementation. This way
>> the run-time lookup is avoided.
>>
>> The xdp_do_redirect() patch restructures the code, so that the map
>> pointer indirection can be avoided.
>>
>> Performance-wise I got 3% improvement for XSKMAP
>> (sample:xdpsock/rx-drop), and 4% (sample:xdp_redirect_map) on my
>> machine.
>>
>> More details in each commit.
>>
>> @Jesper/Toke I dropped your Acked-by: on the first patch, since there
>> were major restucturing. Please have another look! Thanks!
>
> Will do! Did you update the performance numbers above after that change?
>
I did. The XSKMAP performance stayed the same (no surprise, since the
code was the same). However, for the DEVMAP the v4 got rid of a call, so
it *should* be a bit better, but for some reason it didn't show on my
machine.
Björn
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation
2021-02-26 11:37 ` Toke Høiland-Jørgensen
@ 2021-02-26 11:40 ` Björn Töpel
2021-02-26 12:04 ` Björn Töpel
2021-02-26 14:27 ` Jesper Dangaard Brouer
2021-02-26 14:29 ` Jesper Dangaard Brouer
1 sibling, 2 replies; 17+ messages in thread
From: Björn Töpel @ 2021-02-26 11:40 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, Björn Töpel, ast,
daniel, netdev, bpf
Cc: maciej.fijalkowski, hawk, magnus.karlsson, john.fastabend, kuba, davem
On 2021-02-26 12:37, Toke Høiland-Jørgensen wrote:
> Björn Töpel <bjorn.topel@gmail.com> writes:
>
>> From: Björn Töpel <bjorn.topel@intel.com>
>>
>> Currently the bpf_redirect_map() implementation dispatches to the
>> correct map-lookup function via a switch-statement. To avoid the
>> dispatching, this change adds bpf_redirect_map() as a map
>> operation. Each map provides its bpf_redirect_map() version, and
>> correct function is automatically selected by the BPF verifier.
>>
>> A nice side-effect of the code movement is that the map lookup
>> functions are now local to the map implementation files, which removes
>> one additional function call.
>>
>> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
>
> Nice! I agree that this is a much nicer approach! :)
>
> (That last paragraph above is why I asked if you updated the performance
> numbers in the cover letter; removing an additional function call should
> affect those, right?)
>
Yeah, it should. Let me spend some more time benchmarking on the DEVMAP
scenario.
@Jesper Do you have a CPUMAP benchmark that you can point me to? I just
did functional testing for CPUMAP
> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
>
Thank you!
Björn
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 0/2] Optimize bpf_redirect_map()/xdp_do_redirect()
2021-02-26 11:38 ` Björn Töpel
@ 2021-02-26 11:43 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-02-26 11:43 UTC (permalink / raw)
To: Björn Töpel, Björn Töpel, ast, daniel, netdev, bpf
Cc: maciej.fijalkowski, hawk, magnus.karlsson, john.fastabend, kuba, davem
Björn Töpel <bjorn.topel@intel.com> writes:
> On 2021-02-26 12:35, Toke Høiland-Jørgensen wrote:
>> Björn Töpel <bjorn.topel@gmail.com> writes:
>>
>>> Hi XDP-folks,
>>>
>>> This two patch series contain two optimizations for the
>>> bpf_redirect_map() helper and the xdp_do_redirect() function.
>>>
>>> The bpf_redirect_map() optimization is about avoiding the map lookup
>>> dispatching. Instead of having a switch-statement and selecting the
>>> correct lookup function, we let bpf_redirect_map() be a map operation,
>>> where each map has its own bpf_redirect_map() implementation. This way
>>> the run-time lookup is avoided.
>>>
>>> The xdp_do_redirect() patch restructures the code, so that the map
>>> pointer indirection can be avoided.
>>>
>>> Performance-wise I got 3% improvement for XSKMAP
>>> (sample:xdpsock/rx-drop), and 4% (sample:xdp_redirect_map) on my
>>> machine.
>>>
>>> More details in each commit.
>>>
>>> @Jesper/Toke I dropped your Acked-by: on the first patch, since there
>>> were major restucturing. Please have another look! Thanks!
>>
>> Will do! Did you update the performance numbers above after that change?
>>
>
> I did. The XSKMAP performance stayed the same (no surprise, since the
> code was the same). However, for the DEVMAP the v4 got rid of a call, so
> it *should* be a bit better, but for some reason it didn't show on my
> machine.
Alright, fair enough - pesky real world not lining up with expectations!
Maybe Jesper has additional suggestions, but I can live with the 4%
improvement ;)
-Toke
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation
2021-02-26 11:40 ` Björn Töpel
@ 2021-02-26 12:04 ` Björn Töpel
2021-02-26 12:26 ` Toke Høiland-Jørgensen
2021-02-26 14:27 ` Jesper Dangaard Brouer
1 sibling, 1 reply; 17+ messages in thread
From: Björn Töpel @ 2021-02-26 12:04 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, Björn Töpel, ast,
daniel, netdev, bpf
Cc: maciej.fijalkowski, hawk, magnus.karlsson, john.fastabend, kuba, davem
On 2021-02-26 12:40, Björn Töpel wrote:
> On 2021-02-26 12:37, Toke Høiland-Jørgensen wrote:
[...]
>>
>> (That last paragraph above is why I asked if you updated the performance
>> numbers in the cover letter; removing an additional function call should
>> affect those, right?)
>>
>
> Yeah, it should. Let me spend some more time benchmarking on the DEVMAP
> scenario.
>
I did a re-measure using samples/xdp_redirect_map.
The setup is 64B packets blasted to an i40e. As a baseline,
# xdp_rxq_info --dev ens801f1 --action XDP_DROP
gives 24.8 Mpps.
Now, xdp_redirect_map. Same NIC, two ports, receive from port A,
redirect to port B:
baseline: 14.3 Mpps
this series: 15.4 Mpps
which is almost 8%!
Björn
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation
2021-02-26 12:04 ` Björn Töpel
@ 2021-02-26 12:26 ` Toke Høiland-Jørgensen
2021-02-26 14:28 ` Jesper Dangaard Brouer
0 siblings, 1 reply; 17+ messages in thread
From: Toke Høiland-Jørgensen @ 2021-02-26 12:26 UTC (permalink / raw)
To: Björn Töpel, Björn Töpel, ast, daniel, netdev, bpf
Cc: maciej.fijalkowski, hawk, magnus.karlsson, john.fastabend, kuba, davem
Björn Töpel <bjorn.topel@intel.com> writes:
> On 2021-02-26 12:40, Björn Töpel wrote:
>> On 2021-02-26 12:37, Toke Høiland-Jørgensen wrote:
>
> [...]
>
>>>
>>> (That last paragraph above is why I asked if you updated the performance
>>> numbers in the cover letter; removing an additional function call should
>>> affect those, right?)
>>>
>>
>> Yeah, it should. Let me spend some more time benchmarking on the DEVMAP
>> scenario.
>>
>
> I did a re-measure using samples/xdp_redirect_map.
>
> The setup is 64B packets blasted to an i40e. As a baseline,
>
> # xdp_rxq_info --dev ens801f1 --action XDP_DROP
>
> gives 24.8 Mpps.
>
>
> Now, xdp_redirect_map. Same NIC, two ports, receive from port A,
> redirect to port B:
>
> baseline: 14.3 Mpps
> this series: 15.4 Mpps
>
> which is almost 8%!
Or 5 ns difference:
10**9/(14.3*10**6) - 10**9/(15.4*10**6)
4.995004995005004
Nice :)
-Toke
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation
2021-02-26 11:40 ` Björn Töpel
2021-02-26 12:04 ` Björn Töpel
@ 2021-02-26 14:27 ` Jesper Dangaard Brouer
1 sibling, 0 replies; 17+ messages in thread
From: Jesper Dangaard Brouer @ 2021-02-26 14:27 UTC (permalink / raw)
To: Björn Töpel
Cc: Toke Høiland-Jørgensen, Björn Töpel, ast,
daniel, netdev, bpf, maciej.fijalkowski, hawk, magnus.karlsson,
john.fastabend, kuba, davem
On Fri, 26 Feb 2021 12:40:33 +0100
Björn Töpel <bjorn.topel@intel.com> wrote:
> @Jesper Do you have a CPUMAP benchmark that you can point me to? I just
> did functional testing for CPUMAP
I usually just use the xdp_redirect_cpu samples/bpf program.
Your optimization will help the RX enqueue side, but the bottleneck for
CPUMAP is the remote CPU dequeue. You should still be able to see that
RX-side performance improve, and that should be enough (even-though
packets are dropped before reaching remote CPU). I'm not going to ask
you to test scale out to more CPUs.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation
2021-02-26 12:26 ` Toke Høiland-Jørgensen
@ 2021-02-26 14:28 ` Jesper Dangaard Brouer
0 siblings, 0 replies; 17+ messages in thread
From: Jesper Dangaard Brouer @ 2021-02-26 14:28 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Björn Töpel, Björn Töpel, ast, daniel,
netdev, bpf, maciej.fijalkowski, hawk, magnus.karlsson,
john.fastabend, kuba, davem
On Fri, 26 Feb 2021 13:26:22 +0100
Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> Björn Töpel <bjorn.topel@intel.com> writes:
>
> > On 2021-02-26 12:40, Björn Töpel wrote:
> >> On 2021-02-26 12:37, Toke Høiland-Jørgensen wrote:
> >
> > [...]
> >
> >>>
> >>> (That last paragraph above is why I asked if you updated the performance
> >>> numbers in the cover letter; removing an additional function call should
> >>> affect those, right?)
> >>>
> >>
> >> Yeah, it should. Let me spend some more time benchmarking on the DEVMAP
> >> scenario.
> >>
> >
> > I did a re-measure using samples/xdp_redirect_map.
> >
> > The setup is 64B packets blasted to an i40e. As a baseline,
> >
> > # xdp_rxq_info --dev ens801f1 --action XDP_DROP
> >
> > gives 24.8 Mpps.
> >
> >
> > Now, xdp_redirect_map. Same NIC, two ports, receive from port A,
> > redirect to port B:
> >
> > baseline: 14.3 Mpps
> > this series: 15.4 Mpps
> >
> > which is almost 8%!
>
> Or 5 ns difference:
>
> 10**9/(14.3*10**6) - 10**9/(15.4*10**6)
> 4.995004995005004
>
> Nice :)
Yes, this is a very significant improvement at this zoom-in
benchmarking level :-)
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation
2021-02-26 11:37 ` Toke Høiland-Jørgensen
2021-02-26 11:40 ` Björn Töpel
@ 2021-02-26 14:29 ` Jesper Dangaard Brouer
1 sibling, 0 replies; 17+ messages in thread
From: Jesper Dangaard Brouer @ 2021-02-26 14:29 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: brouer, Björn Töpel, ast, daniel, netdev, bpf,
Björn Töpel, maciej.fijalkowski, hawk, magnus.karlsson,
john.fastabend, kuba, davem
On Fri, 26 Feb 2021 12:37:40 +0100
Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> Björn Töpel <bjorn.topel@gmail.com> writes:
>
> > From: Björn Töpel <bjorn.topel@intel.com>
> >
> > Currently the bpf_redirect_map() implementation dispatches to the
> > correct map-lookup function via a switch-statement. To avoid the
> > dispatching, this change adds bpf_redirect_map() as a map
> > operation. Each map provides its bpf_redirect_map() version, and
> > correct function is automatically selected by the BPF verifier.
> >
> > A nice side-effect of the code movement is that the map lookup
> > functions are now local to the map implementation files, which removes
> > one additional function call.
> >
> > Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
>
> Nice! I agree that this is a much nicer approach! :)
I agree :-)
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation
2021-02-26 11:23 ` [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation Björn Töpel
2021-02-26 11:37 ` Toke Høiland-Jørgensen
@ 2021-02-26 15:23 ` kernel test robot
2021-02-26 21:48 ` Daniel Borkmann
2 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2021-02-26 15:23 UTC (permalink / raw)
To: Björn Töpel, ast, daniel, netdev, bpf
Cc: kbuild-all, Björn Töpel, maciej.fijalkowski, hawk,
toke, magnus.karlsson, john.fastabend
[-- Attachment #1: Type: text/plain, Size: 6978 bytes --]
Hi "Björn,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 9c8f21e6f8856a96634e542a58ef3abf27486801]
url: https://github.com/0day-ci/linux/commits/Bj-rn-T-pel/Optimize-bpf_redirect_map-xdp_do_redirect/20210226-192840
base: 9c8f21e6f8856a96634e542a58ef3abf27486801
config: mips-randconfig-r026-20210226 (attached as .config)
compiler: mips64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/1f7606274f17503baf1c0908dad3462981840749
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Bj-rn-T-pel/Optimize-bpf_redirect_map-xdp_do_redirect/20210226-192840
git checkout 1f7606274f17503baf1c0908dad3462981840749
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/bpf_verifier.h:9,
from kernel/bpf/verifier.c:12:
kernel/bpf/verifier.c: In function 'jit_subprogs':
include/linux/filter.h:363:4: warning: cast between incompatible function types from 'unsigned int (*)(const void *, const struct bpf_insn *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type]
363 | ((u64 (*)(u64, u64, u64, u64, u64))(x))
| ^
kernel/bpf/verifier.c:11421:16: note: in expansion of macro 'BPF_CAST_CALL'
11421 | insn->imm = BPF_CAST_CALL(func[subprog]->bpf_func) -
| ^~~~~~~~~~~~~
kernel/bpf/verifier.c: In function 'fixup_bpf_calls':
include/linux/filter.h:363:4: warning: cast between incompatible function types from 'void * (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type]
363 | ((u64 (*)(u64, u64, u64, u64, u64))(x))
| ^
kernel/bpf/verifier.c:11814:17: note: in expansion of macro 'BPF_CAST_CALL'
11814 | insn->imm = BPF_CAST_CALL(ops->map_lookup_elem) -
| ^~~~~~~~~~~~~
include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *, void *, u64)' {aka 'int (* const)(struct bpf_map *, void *, void *, long long unsigned int)'} to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type]
363 | ((u64 (*)(u64, u64, u64, u64, u64))(x))
| ^
kernel/bpf/verifier.c:11818:17: note: in expansion of macro 'BPF_CAST_CALL'
11818 | insn->imm = BPF_CAST_CALL(ops->map_update_elem) -
| ^~~~~~~~~~~~~
include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type]
363 | ((u64 (*)(u64, u64, u64, u64, u64))(x))
| ^
kernel/bpf/verifier.c:11822:17: note: in expansion of macro 'BPF_CAST_CALL'
11822 | insn->imm = BPF_CAST_CALL(ops->map_delete_elem) -
| ^~~~~~~~~~~~~
include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *, u64)' {aka 'int (* const)(struct bpf_map *, void *, long long unsigned int)'} to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type]
363 | ((u64 (*)(u64, u64, u64, u64, u64))(x))
| ^
kernel/bpf/verifier.c:11826:17: note: in expansion of macro 'BPF_CAST_CALL'
11826 | insn->imm = BPF_CAST_CALL(ops->map_push_elem) -
| ^~~~~~~~~~~~~
include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type]
363 | ((u64 (*)(u64, u64, u64, u64, u64))(x))
| ^
kernel/bpf/verifier.c:11830:17: note: in expansion of macro 'BPF_CAST_CALL'
11830 | insn->imm = BPF_CAST_CALL(ops->map_pop_elem) -
| ^~~~~~~~~~~~~
include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, void *)' to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type]
363 | ((u64 (*)(u64, u64, u64, u64, u64))(x))
| ^
kernel/bpf/verifier.c:11834:17: note: in expansion of macro 'BPF_CAST_CALL'
11834 | insn->imm = BPF_CAST_CALL(ops->map_peek_elem) -
| ^~~~~~~~~~~~~
>> include/linux/filter.h:363:4: warning: cast between incompatible function types from 'int (* const)(struct bpf_map *, u32, u64)' {aka 'int (* const)(struct bpf_map *, unsigned int, long long unsigned int)'} to 'u64 (*)(u64, u64, u64, u64, u64)' {aka 'long long unsigned int (*)(long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int, long long unsigned int)'} [-Wcast-function-type]
363 | ((u64 (*)(u64, u64, u64, u64, u64))(x))
| ^
kernel/bpf/verifier.c:11838:17: note: in expansion of macro 'BPF_CAST_CALL'
11838 | insn->imm = BPF_CAST_CALL(ops->xdp_redirect_map) - __bpf_call_base;
| ^~~~~~~~~~~~~
vim +363 include/linux/filter.h
f8f6d679aaa78b Daniel Borkmann 2014-05-29 361
09772d92cd5ad9 Daniel Borkmann 2018-06-02 362 #define BPF_CAST_CALL(x) \
09772d92cd5ad9 Daniel Borkmann 2018-06-02 @363 ((u64 (*)(u64, u64, u64, u64, u64))(x))
09772d92cd5ad9 Daniel Borkmann 2018-06-02 364
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24530 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation
2021-02-26 11:23 ` [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation Björn Töpel
2021-02-26 11:37 ` Toke Høiland-Jørgensen
2021-02-26 15:23 ` kernel test robot
@ 2021-02-26 21:48 ` Daniel Borkmann
2021-02-27 9:04 ` Björn Töpel
2 siblings, 1 reply; 17+ messages in thread
From: Daniel Borkmann @ 2021-02-26 21:48 UTC (permalink / raw)
To: Björn Töpel, ast, netdev, bpf
Cc: Björn Töpel, maciej.fijalkowski, hawk, toke,
magnus.karlsson, john.fastabend, kuba, davem
On 2/26/21 12:23 PM, Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> Currently the bpf_redirect_map() implementation dispatches to the
> correct map-lookup function via a switch-statement. To avoid the
> dispatching, this change adds bpf_redirect_map() as a map
> operation. Each map provides its bpf_redirect_map() version, and
> correct function is automatically selected by the BPF verifier.
>
> A nice side-effect of the code movement is that the map lookup
> functions are now local to the map implementation files, which removes
> one additional function call.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
> include/linux/bpf.h | 26 ++++++--------------------
> include/linux/filter.h | 27 +++++++++++++++++++++++++++
> include/net/xdp_sock.h | 19 -------------------
> kernel/bpf/cpumap.c | 8 +++++++-
> kernel/bpf/devmap.c | 16 ++++++++++++++--
> kernel/bpf/verifier.c | 11 +++++++++--
> net/core/filter.c | 39 +--------------------------------------
> net/xdp/xskmap.c | 18 ++++++++++++++++++
> 8 files changed, 82 insertions(+), 82 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index cccaef1088ea..a44ba904ca37 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -117,6 +117,9 @@ struct bpf_map_ops {
> void *owner, u32 size);
> struct bpf_local_storage __rcu ** (*map_owner_storage_ptr)(void *owner);
>
> + /* XDP helpers.*/
> + int (*xdp_redirect_map)(struct bpf_map *map, u32 ifindex, u64 flags);
> +
> /* map_meta_equal must be implemented for maps that can be
> * used as an inner map. It is a runtime check to ensure
> * an inner map can be inserted to an outer map.
[...]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1dda9d81f12c..96705a49225e 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5409,7 +5409,8 @@ record_func_map(struct bpf_verifier_env *env, struct bpf_call_arg_meta *meta,
> func_id != BPF_FUNC_map_delete_elem &&
> func_id != BPF_FUNC_map_push_elem &&
> func_id != BPF_FUNC_map_pop_elem &&
> - func_id != BPF_FUNC_map_peek_elem)
> + func_id != BPF_FUNC_map_peek_elem &&
> + func_id != BPF_FUNC_redirect_map)
> return 0;
>
> if (map == NULL) {
> @@ -11762,7 +11763,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
> insn->imm == BPF_FUNC_map_delete_elem ||
> insn->imm == BPF_FUNC_map_push_elem ||
> insn->imm == BPF_FUNC_map_pop_elem ||
> - insn->imm == BPF_FUNC_map_peek_elem)) {
> + insn->imm == BPF_FUNC_map_peek_elem ||
> + insn->imm == BPF_FUNC_redirect_map)) {
> aux = &env->insn_aux_data[i + delta];
> if (bpf_map_ptr_poisoned(aux))
> goto patch_call_imm;
> @@ -11804,6 +11806,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
> (int (*)(struct bpf_map *map, void *value))NULL));
> BUILD_BUG_ON(!__same_type(ops->map_peek_elem,
> (int (*)(struct bpf_map *map, void *value))NULL));
> + BUILD_BUG_ON(!__same_type(ops->xdp_redirect_map,
> + (int (*)(struct bpf_map *map, u32 ifindex, u64 flags))NULL));
> patch_map_ops_generic:
> switch (insn->imm) {
> case BPF_FUNC_map_lookup_elem:
> @@ -11830,6 +11834,9 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
> insn->imm = BPF_CAST_CALL(ops->map_peek_elem) -
> __bpf_call_base;
> continue;
> + case BPF_FUNC_redirect_map:
> + insn->imm = BPF_CAST_CALL(ops->xdp_redirect_map) - __bpf_call_base;
Small nit: I would name the generic callback ops->map_redirect so that this is in line with
the general naming convention for the map ops. Otherwise this looks much better, thx!
> + continue;
> }
>
> goto patch_call_imm;
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation
2021-02-26 21:48 ` Daniel Borkmann
@ 2021-02-27 9:04 ` Björn Töpel
2021-02-27 10:22 ` Daniel Borkmann
0 siblings, 1 reply; 17+ messages in thread
From: Björn Töpel @ 2021-02-27 9:04 UTC (permalink / raw)
To: Daniel Borkmann
Cc: maciej.fijalkowski, hawk, toke, magnus.karlsson, john.fastabend,
kuba, davem, Björn Töpel, bpf, netdev, ast
On 2021-02-26 22:48, Daniel Borkmann wrote:
> On 2/26/21 12:23 PM, Björn Töpel wrote:
>> From: Björn Töpel <bjorn.topel@intel.com>
>>
>> Currently the bpf_redirect_map() implementation dispatches to the
>> correct map-lookup function via a switch-statement. To avoid the
>> dispatching, this change adds bpf_redirect_map() as a map
>> operation. Each map provides its bpf_redirect_map() version, and
>> correct function is automatically selected by the BPF verifier.
>>
>> A nice side-effect of the code movement is that the map lookup
>> functions are now local to the map implementation files, which removes
>> one additional function call.
>>
>> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
>> ---
>> include/linux/bpf.h | 26 ++++++--------------------
>> include/linux/filter.h | 27 +++++++++++++++++++++++++++
>> include/net/xdp_sock.h | 19 -------------------
>> kernel/bpf/cpumap.c | 8 +++++++-
>> kernel/bpf/devmap.c | 16 ++++++++++++++--
>> kernel/bpf/verifier.c | 11 +++++++++--
>> net/core/filter.c | 39 +--------------------------------------
>> net/xdp/xskmap.c | 18 ++++++++++++++++++
>> 8 files changed, 82 insertions(+), 82 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index cccaef1088ea..a44ba904ca37 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -117,6 +117,9 @@ struct bpf_map_ops {
>> void *owner, u32 size);
>> struct bpf_local_storage __rcu ** (*map_owner_storage_ptr)(void
>> *owner);
>> + /* XDP helpers.*/
>> + int (*xdp_redirect_map)(struct bpf_map *map, u32 ifindex, u64
>> flags);
>> +
>> /* map_meta_equal must be implemented for maps that can be
>> * used as an inner map. It is a runtime check to ensure
>> * an inner map can be inserted to an outer map.
> [...]
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 1dda9d81f12c..96705a49225e 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -5409,7 +5409,8 @@ record_func_map(struct bpf_verifier_env *env,
>> struct bpf_call_arg_meta *meta,
>> func_id != BPF_FUNC_map_delete_elem &&
>> func_id != BPF_FUNC_map_push_elem &&
>> func_id != BPF_FUNC_map_pop_elem &&
>> - func_id != BPF_FUNC_map_peek_elem)
>> + func_id != BPF_FUNC_map_peek_elem &&
>> + func_id != BPF_FUNC_redirect_map)
>> return 0;
>> if (map == NULL) {
>> @@ -11762,7 +11763,8 @@ static int fixup_bpf_calls(struct
>> bpf_verifier_env *env)
>> insn->imm == BPF_FUNC_map_delete_elem ||
>> insn->imm == BPF_FUNC_map_push_elem ||
>> insn->imm == BPF_FUNC_map_pop_elem ||
>> - insn->imm == BPF_FUNC_map_peek_elem)) {
>> + insn->imm == BPF_FUNC_map_peek_elem ||
>> + insn->imm == BPF_FUNC_redirect_map)) {
>> aux = &env->insn_aux_data[i + delta];
>> if (bpf_map_ptr_poisoned(aux))
>> goto patch_call_imm;
>> @@ -11804,6 +11806,8 @@ static int fixup_bpf_calls(struct
>> bpf_verifier_env *env)
>> (int (*)(struct bpf_map *map, void *value))NULL));
>> BUILD_BUG_ON(!__same_type(ops->map_peek_elem,
>> (int (*)(struct bpf_map *map, void *value))NULL));
>> + BUILD_BUG_ON(!__same_type(ops->xdp_redirect_map,
>> + (int (*)(struct bpf_map *map, u32 ifindex, u64
>> flags))NULL));
>> patch_map_ops_generic:
>> switch (insn->imm) {
>> case BPF_FUNC_map_lookup_elem:
>> @@ -11830,6 +11834,9 @@ static int fixup_bpf_calls(struct
>> bpf_verifier_env *env)
>> insn->imm = BPF_CAST_CALL(ops->map_peek_elem) -
>> __bpf_call_base;
>> continue;
>> + case BPF_FUNC_redirect_map:
>> + insn->imm = BPF_CAST_CALL(ops->xdp_redirect_map) -
>> __bpf_call_base;
>
> Small nit: I would name the generic callback ops->map_redirect so that
> this is in line with
> the general naming convention for the map ops. Otherwise this looks much
> better, thx!
>
I'll respin! Thanks for the input!
I'll ignore the BPF_CAST_CALL W=1 warnings ([-Wcast-function-type]), or
do you have any thoughts on that? I don't think it's a good idea to
silence that warning for the whole verifier.c
Björn
>> + continue;
>> }
>> goto patch_call_imm;
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation
2021-02-27 9:04 ` Björn Töpel
@ 2021-02-27 10:22 ` Daniel Borkmann
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Borkmann @ 2021-02-27 10:22 UTC (permalink / raw)
To: Björn Töpel
Cc: maciej.fijalkowski, hawk, toke, magnus.karlsson, john.fastabend,
kuba, davem, Björn Töpel, bpf, netdev, ast
On 2/27/21 10:04 AM, Björn Töpel wrote:
> On 2021-02-26 22:48, Daniel Borkmann wrote:
>> On 2/26/21 12:23 PM, Björn Töpel wrote:
>>> From: Björn Töpel <bjorn.topel@intel.com>
>>>
>>> Currently the bpf_redirect_map() implementation dispatches to the
>>> correct map-lookup function via a switch-statement. To avoid the
>>> dispatching, this change adds bpf_redirect_map() as a map
>>> operation. Each map provides its bpf_redirect_map() version, and
>>> correct function is automatically selected by the BPF verifier.
>>>
>>> A nice side-effect of the code movement is that the map lookup
>>> functions are now local to the map implementation files, which removes
>>> one additional function call.
>>>
>>> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
>>> ---
>>> include/linux/bpf.h | 26 ++++++--------------------
>>> include/linux/filter.h | 27 +++++++++++++++++++++++++++
>>> include/net/xdp_sock.h | 19 -------------------
>>> kernel/bpf/cpumap.c | 8 +++++++-
>>> kernel/bpf/devmap.c | 16 ++++++++++++++--
>>> kernel/bpf/verifier.c | 11 +++++++++--
>>> net/core/filter.c | 39 +--------------------------------------
>>> net/xdp/xskmap.c | 18 ++++++++++++++++++
>>> 8 files changed, 82 insertions(+), 82 deletions(-)
>>>
>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>> index cccaef1088ea..a44ba904ca37 100644
>>> --- a/include/linux/bpf.h
>>> +++ b/include/linux/bpf.h
>>> @@ -117,6 +117,9 @@ struct bpf_map_ops {
>>> void *owner, u32 size);
>>> struct bpf_local_storage __rcu ** (*map_owner_storage_ptr)(void *owner);
>>> + /* XDP helpers.*/
>>> + int (*xdp_redirect_map)(struct bpf_map *map, u32 ifindex, u64 flags);
>>> +
>>> /* map_meta_equal must be implemented for maps that can be
>>> * used as an inner map. It is a runtime check to ensure
>>> * an inner map can be inserted to an outer map.
>> [...]
>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>> index 1dda9d81f12c..96705a49225e 100644
>>> --- a/kernel/bpf/verifier.c
>>> +++ b/kernel/bpf/verifier.c
>>> @@ -5409,7 +5409,8 @@ record_func_map(struct bpf_verifier_env *env, struct bpf_call_arg_meta *meta,
>>> func_id != BPF_FUNC_map_delete_elem &&
>>> func_id != BPF_FUNC_map_push_elem &&
>>> func_id != BPF_FUNC_map_pop_elem &&
>>> - func_id != BPF_FUNC_map_peek_elem)
>>> + func_id != BPF_FUNC_map_peek_elem &&
>>> + func_id != BPF_FUNC_redirect_map)
>>> return 0;
>>> if (map == NULL) {
>>> @@ -11762,7 +11763,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
>>> insn->imm == BPF_FUNC_map_delete_elem ||
>>> insn->imm == BPF_FUNC_map_push_elem ||
>>> insn->imm == BPF_FUNC_map_pop_elem ||
>>> - insn->imm == BPF_FUNC_map_peek_elem)) {
>>> + insn->imm == BPF_FUNC_map_peek_elem ||
>>> + insn->imm == BPF_FUNC_redirect_map)) {
>>> aux = &env->insn_aux_data[i + delta];
>>> if (bpf_map_ptr_poisoned(aux))
>>> goto patch_call_imm;
>>> @@ -11804,6 +11806,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
>>> (int (*)(struct bpf_map *map, void *value))NULL));
>>> BUILD_BUG_ON(!__same_type(ops->map_peek_elem,
>>> (int (*)(struct bpf_map *map, void *value))NULL));
>>> + BUILD_BUG_ON(!__same_type(ops->xdp_redirect_map,
>>> + (int (*)(struct bpf_map *map, u32 ifindex, u64 flags))NULL));
>>> patch_map_ops_generic:
>>> switch (insn->imm) {
>>> case BPF_FUNC_map_lookup_elem:
>>> @@ -11830,6 +11834,9 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
>>> insn->imm = BPF_CAST_CALL(ops->map_peek_elem) -
>>> __bpf_call_base;
>>> continue;
>>> + case BPF_FUNC_redirect_map:
>>> + insn->imm = BPF_CAST_CALL(ops->xdp_redirect_map) - __bpf_call_base;
>>
>> Small nit: I would name the generic callback ops->map_redirect so that this is in line with
>> the general naming convention for the map ops. Otherwise this looks much better, thx!
>>
>
> I'll respin! Thanks for the input!
>
> I'll ignore the BPF_CAST_CALL W=1 warnings ([-Wcast-function-type]), or
> do you have any thoughts on that? I don't think it's a good idea to
> silence that warning for the whole verifier.c
Makes sense, yes, given they are neither new nor critical for the existing
ones either.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-02-27 10:23 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-26 11:23 [PATCH bpf-next v4 0/2] Optimize bpf_redirect_map()/xdp_do_redirect() Björn Töpel
2021-02-26 11:23 ` [PATCH bpf-next v4 1/2] bpf, xdp: make bpf_redirect_map() a map operation Björn Töpel
2021-02-26 11:37 ` Toke Høiland-Jørgensen
2021-02-26 11:40 ` Björn Töpel
2021-02-26 12:04 ` Björn Töpel
2021-02-26 12:26 ` Toke Høiland-Jørgensen
2021-02-26 14:28 ` Jesper Dangaard Brouer
2021-02-26 14:27 ` Jesper Dangaard Brouer
2021-02-26 14:29 ` Jesper Dangaard Brouer
2021-02-26 15:23 ` kernel test robot
2021-02-26 21:48 ` Daniel Borkmann
2021-02-27 9:04 ` Björn Töpel
2021-02-27 10:22 ` Daniel Borkmann
2021-02-26 11:23 ` [PATCH bpf-next v4 2/2] bpf, xdp: restructure redirect actions Björn Töpel
2021-02-26 11:35 ` [PATCH bpf-next v4 0/2] Optimize bpf_redirect_map()/xdp_do_redirect() Toke Høiland-Jørgensen
2021-02-26 11:38 ` Björn Töpel
2021-02-26 11:43 ` Toke Høiland-Jørgensen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).