| Message ID | 20231006154933.mQgxQHHt@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] net: Add a warning if NAPI cb missed xdp_do_flush(). | expand |
Hi Sebastian, kernel test robot noticed the following build warnings: [auto build test WARNING on bpf-next/master] url: https://github.com/intel-lab-lkp/linux/commits/Sebastian-Andrzej-Siewior/net-Add-a-warning-if-NAPI-cb-missed-xdp_do_flush/20231006-235117 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master patch link: https://lore.kernel.org/r/20231006154933.mQgxQHHt%40linutronix.de patch subject: [PATCH bpf-next v2] net: Add a warning if NAPI cb missed xdp_do_flush(). config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231007/202310070134.JX5try68-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231007/202310070134.JX5try68-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202310070134.JX5try68-lkp@intel.com/ All warnings (new ones prefixed by >>): >> net/core/filter.c:4211:6: warning: no previous prototype for 'xdp_do_check_flushed' [-Wmissing-prototypes] 4211 | void xdp_do_check_flushed(struct napi_struct *napi) | ^~~~~~~~~~~~~~~~~~~~ vim +/xdp_do_check_flushed +4211 net/core/filter.c 4209 4210 #if defined(CONFIG_DEBUG_NET) && defined(CONFIG_BPF_SYSCALL) > 4211 void xdp_do_check_flushed(struct napi_struct *napi) 4212 { 4213 bool ret; 4214 4215 ret = dev_check_flush(); 4216 ret |= cpu_map_check_flush(); 4217 ret |= xsk_map_check_flush(); 4218 4219 WARN_ONCE(ret, "Missing xdp_do_flush() invocation after NAPI by %ps\n", 4220 napi->poll); 4221 } 4222 #endif 4223
On Fri, 6 Oct 2023 17:49:33 +0200 Sebastian Andrzej Siewior wrote: > - Moved xdp_do_check_flushed() to net/core/dev.h. > - Stripped __ from function names. > - Removed empty lines within an ifdef block. > - xdp_do_check_flushed() is now behind CONFIG_DEBUG_NET && > CONFIG_BPF_SYSCALL. dev_check_flush and cpu_map_check_flush are now > only behind CONFIG_DEBUG_NET. They have no empty inline function for > the !CONFIG_DEBUG_NET case since they are only called in > CONFIG_DEBUG_NET case. Other than the minor nit from the build bot - LGTM now thanks! Acked-by: Jakub Kicinski <kuba@kernel.org>
On Fri, Oct 06, 2023 at 05:49:33PM +0200, Sebastian Andrzej Siewior wrote: > 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. nit: creates > Hide everything behind CONFIG_DEBUG_NET. > > Suggested-by: Jesper Dangaard Brouer <hawk@kernel.org> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> ...
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index a34ac7f00c86c..584adabd411fc 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -2478,6 +2478,9 @@ void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data, enum bpf_dynptr_type type, u32 offset, u32 size); void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr); void bpf_dynptr_set_rdonly(struct bpf_dynptr_kern *ptr); + +bool dev_check_flush(void); +bool cpu_map_check_flush(void); #else /* !CONFIG_BPF_SYSCALL */ static inline struct bpf_prog *bpf_prog_get(u32 ufd) { diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h index 69b472604b86f..7dd0df2f6f8e6 100644 --- a/include/net/xdp_sock.h +++ b/include/net/xdp_sock.h @@ -109,4 +109,13 @@ 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..8a0bb80fe48a3 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..a936c704d4e77 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/dev.h b/net/core/dev.h index e075e198092cc..f66125857af77 100644 --- a/net/core/dev.h +++ b/net/core/dev.h @@ -136,4 +136,10 @@ static inline void netif_set_gro_ipv4_max_size(struct net_device *dev, } int rps_cpumask_housekeeping(struct cpumask *mask); + +#if defined(CONFIG_DEBUG_NET) && defined(CONFIG_BPF_SYSCALL) +void xdp_do_check_flushed(struct napi_struct *napi); +#else +static inline void xdp_do_check_flushed(struct napi_struct *napi) { } +#endif #endif diff --git a/net/core/filter.c b/net/core/filter.c index a094694899c99..5d5921c0ab4a3 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); +#if defined(CONFIG_DEBUG_NET) && defined(CONFIG_BPF_SYSCALL) +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 f5e96e0d6e01d..ba070fd37d244 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> --- v1…v2: - Moved xdp_do_check_flushed() to net/core/dev.h. - Stripped __ from function names. - Removed empty lines within an ifdef block. - xdp_do_check_flushed() is now behind CONFIG_DEBUG_NET && CONFIG_BPF_SYSCALL. dev_check_flush and cpu_map_check_flush are now only behind CONFIG_DEBUG_NET. They have no empty inline function for the !CONFIG_DEBUG_NET case since they are only called in CONFIG_DEBUG_NET case. include/linux/bpf.h | 3 +++ include/net/xdp_sock.h | 9 +++++++++ kernel/bpf/cpumap.c | 10 ++++++++++ kernel/bpf/devmap.c | 10 ++++++++++ net/core/dev.c | 2 ++ net/core/dev.h | 6 ++++++ net/core/filter.c | 14 ++++++++++++++ net/xdp/xsk.c | 10 ++++++++++ 8 files changed, 64 insertions(+)