| Message ID | 20230929165825.RvwBYGP1@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] net: Add a warning if NAPI cb missed xdp_do_flush(). | expand |
Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > A few drivers were missing a xdp_do_flush() invocation after > XDP_REDIRECT. > > Add three helper functions each for one of the per-CPU lists. Return > true if the per-CPU list is non-empty and flush the list. > Add xdp_do_check_flushed() which invokes each helper functions and > creats a warning if one of the functions had a non-empty list. > Hide everything behind CONFIG_DEBUG_NET. > > Suggested-by: Jesper Dangaard Brouer <hawk@kernel.org> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > > This is follow-up to > https://lore.kernel.org/all/cb2f7931-5ae5-8583-acff-4a186fed6632@kernel.org > > It has been compile tested. Both with and without CONFIG_DEBUG_NET enabled, I trust? ;) Anyway, LGTM! Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
On Fri, 29 Sep 2023 18:58:25 +0200 Sebastian Andrzej Siewior wrote: > +#if defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_DEBUG_NET) > +bool __dev_check_flush(void); > +bool __cpu_map_check_flush(void); > + > +#else > +static inline bool __dev_check_flush(void) > +{ > + return false; > +} > + > +static inline bool __cpu_map_check_flush(void) > +{ > + return false; > +} > +#endif I think you're going too hard with the ifdefs. These functions are only called if DEBUG_NET, add if BPF on the call site and spare us the static inlines for all the __ helpers. > static __always_inline int > bpf_probe_read_kernel_common(void *dst, u32 size, const void *unsafe_ptr) > { > diff --git a/include/linux/filter.h b/include/linux/filter.h > index 27406aee2d402..db095d731813e 100644 > --- a/include/linux/filter.h > +++ b/include/linux/filter.h > @@ -1025,6 +1025,14 @@ int xdp_do_redirect_frame(struct net_device *dev, > struct bpf_prog *prog); > void xdp_do_flush(void); > > +#ifdef CONFIG_DEBUG_NET > +void xdp_do_check_flushed(struct napi_struct *napi); > + > +#else > +static inline void xdp_do_check_flushed(struct napi_struct *napi) { } > + > +#endif Can you move this to net/core/dev.h? Or a new header under net/core if you prefer? This looks internal to the stack. Also nit: drop the empty lines inside the #ifdef?
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 30063a760b5af..a4eb8f23d35e6 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -2730,6 +2730,22 @@ static inline void bpf_dynptr_set_rdonly(struct bpf_dynptr_kern *ptr) } #endif /* CONFIG_BPF_SYSCALL */ +#if defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_DEBUG_NET) +bool __dev_check_flush(void); +bool __cpu_map_check_flush(void); + +#else +static inline bool __dev_check_flush(void) +{ + return false; +} + +static inline bool __cpu_map_check_flush(void) +{ + return false; +} +#endif + static __always_inline int bpf_probe_read_kernel_common(void *dst, u32 size, const void *unsafe_ptr) { diff --git a/include/linux/filter.h b/include/linux/filter.h index 27406aee2d402..db095d731813e 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -1025,6 +1025,14 @@ int xdp_do_redirect_frame(struct net_device *dev, struct bpf_prog *prog); void xdp_do_flush(void); +#ifdef CONFIG_DEBUG_NET +void xdp_do_check_flushed(struct napi_struct *napi); + +#else +static inline void xdp_do_check_flushed(struct napi_struct *napi) { } + +#endif + /* The xdp_do_flush_map() helper has been renamed to drop the _map suffix, as * it is no longer only flushing maps. Keep this define for compatibility * until all drivers are updated - do not use xdp_do_flush_map() in new code! diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h index 69b472604b86f..c250b78712771 100644 --- a/include/net/xdp_sock.h +++ b/include/net/xdp_sock.h @@ -109,4 +109,14 @@ static inline void __xsk_map_flush(void) #endif /* CONFIG_XDP_SOCKETS */ +#if defined(CONFIG_XDP_SOCKETS) && defined(CONFIG_DEBUG_NET) +bool __xsk_map_check_flush(void); + +#else +static inline bool __xsk_map_check_flush(void) +{ + return false; +} +#endif + #endif /* _LINUX_XDP_SOCK_H */ diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c index e42a1bdb7f536..2cded02d83815 100644 --- a/kernel/bpf/cpumap.c +++ b/kernel/bpf/cpumap.c @@ -764,6 +764,16 @@ void __cpu_map_flush(void) } } +#ifdef CONFIG_DEBUG_NET +bool __cpu_map_check_flush(void) +{ + if (list_empty(this_cpu_ptr(&cpu_map_flush_list))) + return false; + __cpu_map_flush(); + return true; +} +#endif + static int __init cpu_map_init(void) { int cpu; diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c index 4d42f6ed6c11a..8619ac1b879ed 100644 --- a/kernel/bpf/devmap.c +++ b/kernel/bpf/devmap.c @@ -418,6 +418,16 @@ void __dev_flush(void) } } +#ifdef CONFIG_DEBUG_NET +bool __dev_check_flush(void) +{ + if (list_empty(this_cpu_ptr(&dev_flush_list))) + return false; + __dev_flush(); + return true; +} +#endif + /* Elements are kept alive by RCU; either by rcu_read_lock() (from syscall) or * by local_bh_disable() (from XDP calls inside NAPI). The * rcu_read_lock_bh_held() below makes lockdep accept both. diff --git a/net/core/dev.c b/net/core/dev.c index 606a366cc2095..9273b12ecf6fa 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6526,6 +6526,8 @@ static int __napi_poll(struct napi_struct *n, bool *repoll) if (test_bit(NAPI_STATE_SCHED, &n->state)) { work = n->poll(n, weight); trace_napi_poll(n, work, weight); + + xdp_do_check_flushed(n); } if (unlikely(work > weight)) diff --git a/net/core/filter.c b/net/core/filter.c index a094694899c99..9841d0e32cb94 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -4207,6 +4207,20 @@ void xdp_do_flush(void) } EXPORT_SYMBOL_GPL(xdp_do_flush); +#ifdef CONFIG_DEBUG_NET +void xdp_do_check_flushed(struct napi_struct *napi) +{ + bool ret; + + ret = __dev_check_flush(); + ret |= __cpu_map_check_flush(); + ret |= __xsk_map_check_flush(); + + WARN_ONCE(ret, "Missing xdp_do_flush() invocation after NAPI by %ps\n", + napi->poll); +} +#endif + void bpf_clear_redirect_map(struct bpf_map *map) { struct bpf_redirect_info *ri; diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c index 7482d0aca5046..4eae53478db07 100644 --- a/net/xdp/xsk.c +++ b/net/xdp/xsk.c @@ -391,6 +391,16 @@ void __xsk_map_flush(void) } } +#ifdef CONFIG_DEBUG_NET +bool __xsk_map_check_flush(void) +{ + if (list_empty(this_cpu_ptr(&xskmap_flush_list))) + return false; + __xsk_map_flush(); + return true; +} +#endif + void xsk_tx_completed(struct xsk_buff_pool *pool, u32 nb_entries) { xskq_prod_submit_n(pool->cq, nb_entries);
A few drivers were missing a xdp_do_flush() invocation after XDP_REDIRECT. Add three helper functions each for one of the per-CPU lists. Return true if the per-CPU list is non-empty and flush the list. Add xdp_do_check_flushed() which invokes each helper functions and creats a warning if one of the functions had a non-empty list. Hide everything behind CONFIG_DEBUG_NET. Suggested-by: Jesper Dangaard Brouer <hawk@kernel.org> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- This is follow-up to https://lore.kernel.org/all/cb2f7931-5ae5-8583-acff-4a186fed6632@kernel.org It has been compile tested. include/linux/bpf.h | 16 ++++++++++++++++ include/linux/filter.h | 8 ++++++++ include/net/xdp_sock.h | 10 ++++++++++ kernel/bpf/cpumap.c | 10 ++++++++++ kernel/bpf/devmap.c | 10 ++++++++++ net/core/dev.c | 2 ++ net/core/filter.c | 14 ++++++++++++++ net/xdp/xsk.c | 10 ++++++++++ 8 files changed, 80 insertions(+)