Message ID | 20230330151758.531170-4-aditi.ghag@isovalent.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | BPF |
Headers | show |
Series | bpf: Add socket destroy capability | expand |
Hi Aditi, Thank you for the patch! Yet something to improve: [auto build test ERROR on bpf-next/master] url: https://github.com/intel-lab-lkp/linux/commits/Aditi-Ghag/bpf-tcp-Avoid-taking-fast-sock-lock-in-iterator/20230330-232137 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master patch link: https://lore.kernel.org/r/20230330151758.531170-4-aditi.ghag%40isovalent.com patch subject: [PATCH v5 bpf-next 3/7] udp: seq_file: Helper function to match socket attributes config: nios2-defconfig (https://download.01.org/0day-ci/archive/20230331/202303310212.8DXGg50J-lkp@intel.com/config) compiler: nios2-linux-gcc (GCC) 12.1.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/intel-lab-lkp/linux/commit/66cc617bebf6cb3d2162587d6e248d86bc59c1c2 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Aditi-Ghag/bpf-tcp-Avoid-taking-fast-sock-lock-in-iterator/20230330-232137 git checkout 66cc617bebf6cb3d2162587d6e248d86bc59c1c2 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 SHELL=/bin/bash If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202303310212.8DXGg50J-lkp@intel.com/ All errors (new ones prefixed by >>): nios2-linux-ld: net/ipv4/udp.o: in function `udp_get_first': >> net/ipv4/udp.c:3015: undefined reference to `seq_sk_match' net/ipv4/udp.c:3015:(.text+0x4a8): relocation truncated to fit: R_NIOS2_CALL26 against `seq_sk_match' nios2-linux-ld: net/ipv4/udp.o: in function `udp_get_next': net/ipv4/udp.c:3036: undefined reference to `seq_sk_match' net/ipv4/udp.c:3036:(.text+0x55c): relocation truncated to fit: R_NIOS2_CALL26 against `seq_sk_match' vim +3015 net/ipv4/udp.c 2993 2994 static struct sock *udp_get_first(struct seq_file *seq, int start) 2995 { 2996 struct udp_iter_state *state = seq->private; 2997 struct net *net = seq_file_net(seq); 2998 struct udp_seq_afinfo *afinfo; 2999 struct udp_table *udptable; 3000 struct sock *sk; 3001 3002 afinfo = pde_data(file_inode(seq->file)); 3003 3004 udptable = udp_get_table_afinfo(afinfo, net); 3005 3006 for (state->bucket = start; state->bucket <= udptable->mask; 3007 ++state->bucket) { 3008 struct udp_hslot *hslot = &udptable->hash[state->bucket]; 3009 3010 if (hlist_empty(&hslot->head)) 3011 continue; 3012 3013 spin_lock_bh(&hslot->lock); 3014 sk_for_each(sk, &hslot->head) { > 3015 if (seq_sk_match(seq, sk)) 3016 goto found; 3017 } 3018 spin_unlock_bh(&hslot->lock); 3019 } 3020 sk = NULL; 3021 found: 3022 return sk; 3023 } 3024
Hi Aditi, Thank you for the patch! Yet something to improve: [auto build test ERROR on bpf-next/master] url: https://github.com/intel-lab-lkp/linux/commits/Aditi-Ghag/bpf-tcp-Avoid-taking-fast-sock-lock-in-iterator/20230330-232137 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master patch link: https://lore.kernel.org/r/20230330151758.531170-4-aditi.ghag%40isovalent.com patch subject: [PATCH v5 bpf-next 3/7] udp: seq_file: Helper function to match socket attributes config: parisc-defconfig (https://download.01.org/0day-ci/archive/20230331/202303310242.QYEDFSaE-lkp@intel.com/config) compiler: hppa-linux-gcc (GCC) 12.1.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/intel-lab-lkp/linux/commit/66cc617bebf6cb3d2162587d6e248d86bc59c1c2 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Aditi-Ghag/bpf-tcp-Avoid-taking-fast-sock-lock-in-iterator/20230330-232137 git checkout 66cc617bebf6cb3d2162587d6e248d86bc59c1c2 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202303310242.QYEDFSaE-lkp@intel.com/ All error/warnings (new ones prefixed by >>): >> net/ipv4/udp.c:2986:20: warning: 'seq_sk_match' used but never defined 2986 | static inline bool seq_sk_match(struct seq_file *seq, const struct sock *sk); | ^~~~~~~~~~~~ -- hppa-linux-ld: net/ipv4/udp.o: in function `udp_get_first': >> (.text+0x11b0): undefined reference to `seq_sk_match' hppa-linux-ld: net/ipv4/udp.o: in function `udp_get_next': (.text+0x123c): undefined reference to `seq_sk_match'
Hi Aditi, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on bpf-next/master] url: https://github.com/intel-lab-lkp/linux/commits/Aditi-Ghag/bpf-tcp-Avoid-taking-fast-sock-lock-in-iterator/20230330-232137 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master patch link: https://lore.kernel.org/r/20230330151758.531170-4-aditi.ghag%40isovalent.com patch subject: [PATCH v5 bpf-next 3/7] udp: seq_file: Helper function to match socket attributes config: i386-randconfig-a006 (https://download.01.org/0day-ci/archive/20230331/202303311022.2GVz6yAt-lkp@intel.com/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1) 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/intel-lab-lkp/linux/commit/66cc617bebf6cb3d2162587d6e248d86bc59c1c2 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Aditi-Ghag/bpf-tcp-Avoid-taking-fast-sock-lock-in-iterator/20230330-232137 git checkout 66cc617bebf6cb3d2162587d6e248d86bc59c1c2 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash net/ipv4/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202303311022.2GVz6yAt-lkp@intel.com/ All warnings (new ones prefixed by >>): >> net/ipv4/udp.c:2986:20: warning: function 'seq_sk_match' has internal linkage but is not defined [-Wundefined-internal] static inline bool seq_sk_match(struct seq_file *seq, const struct sock *sk); ^ net/ipv4/udp.c:3015:8: note: used here if (seq_sk_match(seq, sk)) ^ 1 warning generated. vim +/seq_sk_match +2986 net/ipv4/udp.c 2985 > 2986 static inline bool seq_sk_match(struct seq_file *seq, const struct sock *sk); 2987
On 3/30/23 8:17 AM, Aditi Ghag wrote: > This is a preparatory commit to refactor code that matches socket > attributes in iterators to a helper function, and use it in the > proc fs iterator. > > Signed-off-by: Aditi Ghag <aditi.ghag@isovalent.com> > --- > net/ipv4/udp.c | 35 ++++++++++++++++++++++++++++------- > 1 file changed, 28 insertions(+), 7 deletions(-) > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > index c574c8c17ec9..cead4acb64c6 100644 > --- a/net/ipv4/udp.c > +++ b/net/ipv4/udp.c > @@ -2983,6 +2983,8 @@ EXPORT_SYMBOL(udp_prot); > /* ------------------------------------------------------------------------ */ > #ifdef CONFIG_PROC_FS > > +static inline bool seq_sk_match(struct seq_file *seq, const struct sock *sk); > + > static struct udp_table *udp_get_table_afinfo(struct udp_seq_afinfo *afinfo, > struct net *net) > { > @@ -3010,10 +3012,7 @@ static struct sock *udp_get_first(struct seq_file *seq, int start) > > spin_lock_bh(&hslot->lock); > sk_for_each(sk, &hslot->head) { > - if (!net_eq(sock_net(sk), net)) > - continue; > - if (afinfo->family == AF_UNSPEC || > - sk->sk_family == afinfo->family) > + if (seq_sk_match(seq, sk)) > goto found; > } > spin_unlock_bh(&hslot->lock); > @@ -3034,9 +3033,7 @@ static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk) > > do { > sk = sk_next(sk); > - } while (sk && (!net_eq(sock_net(sk), net) || > - (afinfo->family != AF_UNSPEC && > - sk->sk_family != afinfo->family))); > + } while (sk && !seq_sk_match(seq, sk)); > > if (!sk) { > udptable = udp_get_table_afinfo(afinfo, net); > @@ -3143,6 +3140,17 @@ struct bpf_iter__udp { > int bucket __aligned(8); > }; > > +static unsigned short seq_file_family(const struct seq_file *seq); > + > +static inline bool seq_sk_match(struct seq_file *seq, const struct sock *sk) This won't work under CONFIG_BPF_SYSCALL. The bot also complained. It has to be under CONFIG_PROG_FS. No need to use inline and leave it to compiler. The earlier forward declaration is unnecessary. Define the function earlier instead. > +{ > + unsigned short family = seq_file_family(seq); > + > + /* AF_UNSPEC is used as a match all */ > + return ((family == AF_UNSPEC || family == sk->sk_family) && > + net_eq(sock_net(sk), seq_file_net(seq))); > +} > + > static int udp_prog_seq_show(struct bpf_prog *prog, struct bpf_iter_meta *meta, > struct udp_sock *udp_sk, uid_t uid, int bucket) > { > @@ -3194,6 +3202,19 @@ static const struct seq_operations bpf_iter_udp_seq_ops = { > .stop = bpf_iter_udp_seq_stop, > .show = bpf_iter_udp_seq_show, > }; > + > +static unsigned short seq_file_family(const struct seq_file *seq) The same here because seq_sk_match() uses seq_file_family(). > +{ > + const struct udp_seq_afinfo *afinfo; > + > + /* BPF iterator: bpf programs to filter sockets. */ > + if (seq->op == &bpf_iter_udp_seq_ops) Note that bpf_iter_udp_seq_ops is only defined under CONFIG_BPF_SYSCALL though. See how the seq_file_family() is handling it in tcp_ipv4.c. > + return AF_UNSPEC; > + > + /* Proc fs iterator */ > + afinfo = pde_data(file_inode(seq->file)); > + return afinfo->family; > +} > #endif > > const struct seq_operations udp_seq_ops = {
Hi Aditi, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on bpf-next/master] url: https://github.com/intel-lab-lkp/linux/commits/Aditi-Ghag/bpf-tcp-Avoid-taking-fast-sock-lock-in-iterator/20230330-232137 base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master patch link: https://lore.kernel.org/r/20230330151758.531170-4-aditi.ghag%40isovalent.com patch subject: [PATCH v5 bpf-next 3/7] udp: seq_file: Helper function to match socket attributes config: i386-randconfig-s001 (https://download.01.org/0day-ci/archive/20230402/202304021454.snASElSi-lkp@intel.com/config) compiler: gcc-11 (Debian 11.3.0-8) 11.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/66cc617bebf6cb3d2162587d6e248d86bc59c1c2 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Aditi-Ghag/bpf-tcp-Avoid-taking-fast-sock-lock-in-iterator/20230330-232137 git checkout 66cc617bebf6cb3d2162587d6e248d86bc59c1c2 # save the config file mkdir build_dir && cp config build_dir/.config make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 olddefconfig make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash net/ipv4/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202304021454.snASElSi-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) net/ipv4/udp.c:1487:28: sparse: sparse: context imbalance in 'udp_rmem_release' - unexpected unlock net/ipv4/udp.c:1519:19: sparse: sparse: context imbalance in 'busylock_acquire' - wrong count at exit net/ipv4/udp.c:1531:28: sparse: sparse: context imbalance in 'busylock_release' - unexpected unlock >> net/ipv4/udp.c:2986:32: sparse: sparse: marked inline, but without a definition >> net/ipv4/udp.c:2986:32: sparse: sparse: marked inline, but without a definition vim +2986 net/ipv4/udp.c 2985 > 2986 static inline bool seq_sk_match(struct seq_file *seq, const struct sock *sk); 2987
> On Mar 31, 2023, at 1:09 PM, Martin KaFai Lau <martin.lau@linux.dev> wrote: > > On 3/30/23 8:17 AM, Aditi Ghag wrote: >> This is a preparatory commit to refactor code that matches socket >> attributes in iterators to a helper function, and use it in the >> proc fs iterator. >> Signed-off-by: Aditi Ghag <aditi.ghag@isovalent.com> >> --- >> net/ipv4/udp.c | 35 ++++++++++++++++++++++++++++------- >> 1 file changed, 28 insertions(+), 7 deletions(-) >> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c >> index c574c8c17ec9..cead4acb64c6 100644 >> --- a/net/ipv4/udp.c >> +++ b/net/ipv4/udp.c >> @@ -2983,6 +2983,8 @@ EXPORT_SYMBOL(udp_prot); >> /* ------------------------------------------------------------------------ */ >> #ifdef CONFIG_PROC_FS >> +static inline bool seq_sk_match(struct seq_file *seq, const struct sock *sk); >> + >> static struct udp_table *udp_get_table_afinfo(struct udp_seq_afinfo *afinfo, >> struct net *net) >> { >> @@ -3010,10 +3012,7 @@ static struct sock *udp_get_first(struct seq_file *seq, int start) >> spin_lock_bh(&hslot->lock); >> sk_for_each(sk, &hslot->head) { >> - if (!net_eq(sock_net(sk), net)) >> - continue; >> - if (afinfo->family == AF_UNSPEC || >> - sk->sk_family == afinfo->family) >> + if (seq_sk_match(seq, sk)) >> goto found; >> } >> spin_unlock_bh(&hslot->lock); >> @@ -3034,9 +3033,7 @@ static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk) >> do { >> sk = sk_next(sk); >> - } while (sk && (!net_eq(sock_net(sk), net) || >> - (afinfo->family != AF_UNSPEC && >> - sk->sk_family != afinfo->family))); >> + } while (sk && !seq_sk_match(seq, sk)); >> if (!sk) { >> udptable = udp_get_table_afinfo(afinfo, net); >> @@ -3143,6 +3140,17 @@ struct bpf_iter__udp { >> int bucket __aligned(8); >> }; >> +static unsigned short seq_file_family(const struct seq_file *seq); >> + >> +static inline bool seq_sk_match(struct seq_file *seq, const struct sock *sk) > > This won't work under CONFIG_BPF_SYSCALL. The bot also complained. > It has to be under CONFIG_PROG_FS. > > No need to use inline and leave it to compiler. > > The earlier forward declaration is unnecessary. Define the function earlier instead. Yes, I got notifications from the kernel test bot, so I've already made the necessary changes. > >> +{ >> + unsigned short family = seq_file_family(seq); >> + >> + /* AF_UNSPEC is used as a match all */ >> + return ((family == AF_UNSPEC || family == sk->sk_family) && >> + net_eq(sock_net(sk), seq_file_net(seq))); >> +} >> + >> static int udp_prog_seq_show(struct bpf_prog *prog, struct bpf_iter_meta *meta, >> struct udp_sock *udp_sk, uid_t uid, int bucket) >> { >> @@ -3194,6 +3202,19 @@ static const struct seq_operations bpf_iter_udp_seq_ops = { >> .stop = bpf_iter_udp_seq_stop, >> .show = bpf_iter_udp_seq_show, >> }; >> + >> +static unsigned short seq_file_family(const struct seq_file *seq) > > The same here because seq_sk_match() uses seq_file_family(). > >> +{ >> + const struct udp_seq_afinfo *afinfo; >> + >> + /* BPF iterator: bpf programs to filter sockets. */ >> + if (seq->op == &bpf_iter_udp_seq_ops) > > Note that bpf_iter_udp_seq_ops is only defined under CONFIG_BPF_SYSCALL though. > See how the seq_file_family() is handling it in tcp_ipv4.c. Same as above. > >> + return AF_UNSPEC; >> + >> + /* Proc fs iterator */ >> + afinfo = pde_data(file_inode(seq->file)); >> + return afinfo->family; >> +} >> #endif >> const struct seq_operations udp_seq_ops = {
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index c574c8c17ec9..cead4acb64c6 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -2983,6 +2983,8 @@ EXPORT_SYMBOL(udp_prot); /* ------------------------------------------------------------------------ */ #ifdef CONFIG_PROC_FS +static inline bool seq_sk_match(struct seq_file *seq, const struct sock *sk); + static struct udp_table *udp_get_table_afinfo(struct udp_seq_afinfo *afinfo, struct net *net) { @@ -3010,10 +3012,7 @@ static struct sock *udp_get_first(struct seq_file *seq, int start) spin_lock_bh(&hslot->lock); sk_for_each(sk, &hslot->head) { - if (!net_eq(sock_net(sk), net)) - continue; - if (afinfo->family == AF_UNSPEC || - sk->sk_family == afinfo->family) + if (seq_sk_match(seq, sk)) goto found; } spin_unlock_bh(&hslot->lock); @@ -3034,9 +3033,7 @@ static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk) do { sk = sk_next(sk); - } while (sk && (!net_eq(sock_net(sk), net) || - (afinfo->family != AF_UNSPEC && - sk->sk_family != afinfo->family))); + } while (sk && !seq_sk_match(seq, sk)); if (!sk) { udptable = udp_get_table_afinfo(afinfo, net); @@ -3143,6 +3140,17 @@ struct bpf_iter__udp { int bucket __aligned(8); }; +static unsigned short seq_file_family(const struct seq_file *seq); + +static inline bool seq_sk_match(struct seq_file *seq, const struct sock *sk) +{ + unsigned short family = seq_file_family(seq); + + /* AF_UNSPEC is used as a match all */ + return ((family == AF_UNSPEC || family == sk->sk_family) && + net_eq(sock_net(sk), seq_file_net(seq))); +} + static int udp_prog_seq_show(struct bpf_prog *prog, struct bpf_iter_meta *meta, struct udp_sock *udp_sk, uid_t uid, int bucket) { @@ -3194,6 +3202,19 @@ static const struct seq_operations bpf_iter_udp_seq_ops = { .stop = bpf_iter_udp_seq_stop, .show = bpf_iter_udp_seq_show, }; + +static unsigned short seq_file_family(const struct seq_file *seq) +{ + const struct udp_seq_afinfo *afinfo; + + /* BPF iterator: bpf programs to filter sockets. */ + if (seq->op == &bpf_iter_udp_seq_ops) + return AF_UNSPEC; + + /* Proc fs iterator */ + afinfo = pde_data(file_inode(seq->file)); + return afinfo->family; +} #endif const struct seq_operations udp_seq_ops = {
This is a preparatory commit to refactor code that matches socket attributes in iterators to a helper function, and use it in the proc fs iterator. Signed-off-by: Aditi Ghag <aditi.ghag@isovalent.com> --- net/ipv4/udp.c | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-)