| Message ID | 20190515205501.17810-1-jakub.kicinski@netronome.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] rhashtable: fix sparse RCU warnings on bit lock in bucket pointer | expand |
On Wed, May 15 2019, Jakub Kicinski wrote: > Since the bit_spin_lock() operations don't actually dereference > the pointer, it's fine to forcefully drop the RCU annotation. > This fixes 7 sparse warnings per include site. > > Fixes: 8f0db018006a ("rhashtable: use bit_spin_locks to protect hash bucket.") > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > Reviewed-by: Simon Horman <simon.horman@netronome.com> Hi, sorry for not responding to your initial post, but I'm otherwise engaged this week and cannot give it any real time. I don't object to this patch, but I'll try to have a proper look next week, if only to find out how I didn't get the warnings, as I was testing with sparse. Thanks, NeilBrown > --- > include/linux/rhashtable.h | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h > index f7714d3b46bd..bea1e0440ab4 100644 > --- a/include/linux/rhashtable.h > +++ b/include/linux/rhashtable.h > @@ -325,27 +325,27 @@ static inline struct rhash_lock_head __rcu **rht_bucket_insert( > */ > > static inline void rht_lock(struct bucket_table *tbl, > - struct rhash_lock_head **bkt) > + struct rhash_lock_head __rcu **bkt) > { > local_bh_disable(); > - bit_spin_lock(0, (unsigned long *)bkt); > + bit_spin_lock(0, (unsigned long __force *)bkt); > lock_map_acquire(&tbl->dep_map); > } > > static inline void rht_lock_nested(struct bucket_table *tbl, > - struct rhash_lock_head **bucket, > + struct rhash_lock_head __rcu **bkt, > unsigned int subclass) > { > local_bh_disable(); > - bit_spin_lock(0, (unsigned long *)bucket); > + bit_spin_lock(0, (unsigned long __force *)bkt); > lock_acquire_exclusive(&tbl->dep_map, subclass, 0, NULL, _THIS_IP_); > } > > static inline void rht_unlock(struct bucket_table *tbl, > - struct rhash_lock_head **bkt) > + struct rhash_lock_head __rcu **bkt) > { > lock_map_release(&tbl->dep_map); > - bit_spin_unlock(0, (unsigned long *)bkt); > + bit_spin_unlock(0, (unsigned long __force *)bkt); > local_bh_enable(); > } > > -- > 2.21.0
On Thu, 16 May 2019 07:42:29 +1000, NeilBrown wrote: > On Wed, May 15 2019, Jakub Kicinski wrote: > > > Since the bit_spin_lock() operations don't actually dereference > > the pointer, it's fine to forcefully drop the RCU annotation. > > This fixes 7 sparse warnings per include site. > > > > Fixes: 8f0db018006a ("rhashtable: use bit_spin_locks to protect hash bucket.") > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > > Reviewed-by: Simon Horman <simon.horman@netronome.com> > > Hi, sorry for not responding to your initial post, but I'm otherwise > engaged this week and cannot give it any real time. I don't object to > this patch, but I'll try to have a proper look next week, if only to > find out how I didn't get the warnings, as I was testing with sparse. You gave me a scare :) I pulled latest sparse and they seem to still be there (previously I was testing with sparse 5.2). Note that I'm just fixing the warnings in the header, those are particularly noisy as they get printed for each include site. $ gcc-9 --version gcc-9 (Ubuntu 9-20190428-1ubuntu1~18.04.york0) 9.0.1 20190428 (prerelease) [gcc-9-branch revision 270630] [...] $ sparse --version v0.6.1-rc1-7-g2b96cd80 $ git checkout net/master $ make CC=gcc-9 O=build_net-perf/ -j 32 W=1 C=1 make[1]: Entering directory '/home/jkicinski/devel/linux/build_net-perf' GEN Makefile DESCEND objtool Using .. as source for kernel CALL ../scripts/atomic/check-atomics.sh CALL ../scripts/checksyscalls.sh CHK include/generated/compile.h CHECK ../lib/rhashtable.c ../lib/rhashtable.c:134:13: warning: incorrect type in initializer (different address spaces) ../lib/rhashtable.c:134:13: expected union nested_table [noderef] <asn:4> *__new ../lib/rhashtable.c:134:13: got union nested_table *[assigned] ntbl ../lib/rhashtable.c:250:51: warning: incorrect type in argument 2 (different address spaces) ../lib/rhashtable.c:250:51: expected struct rhash_lock_head **bucket ../lib/rhashtable.c:250:51: got struct rhash_lock_head [noderef] <asn:4> ** ../lib/rhashtable.c:277:27: warning: incorrect type in argument 2 (different address spaces) ../lib/rhashtable.c:277:27: expected struct rhash_lock_head **bkt ../lib/rhashtable.c:277:27: got struct rhash_lock_head [noderef] <asn:4> **bkt ../lib/rhashtable.c:284:29: warning: incorrect type in argument 2 (different address spaces) ../lib/rhashtable.c:284:29: expected struct rhash_lock_head **bkt ../lib/rhashtable.c:284:29: got struct rhash_lock_head [noderef] <asn:4> **bkt ../lib/rhashtable.c:299:13: warning: incorrect type in initializer (different address spaces) ../lib/rhashtable.c:299:13: expected struct bucket_table [noderef] <asn:4> *__new ../lib/rhashtable.c:299:13: got struct bucket_table *new_tbl ../lib/rhashtable.c:605:39: warning: incorrect type in argument 2 (different address spaces) ../lib/rhashtable.c:605:39: expected struct rhash_lock_head **bkt ../lib/rhashtable.c:605:39: got struct rhash_lock_head [noderef] <asn:4> **[assigned] bkt ../lib/rhashtable.c:613:41: warning: incorrect type in argument 2 (different address spaces) ../lib/rhashtable.c:613:41: expected struct rhash_lock_head **bkt ../lib/rhashtable.c:613:41: got struct rhash_lock_head [noderef] <asn:4> **[assigned] bkt [...] $ git checkout rht-fix $ make CC=gcc-9 O=build_net-perf/ -j 32 W=1 C=1 [...all things get rebuilt...] $ touch lib/rhashtable.c $ make CC=gcc-9 O=build_net-perf/ -j 32 W=1 C=1 make[1]: Entering directory '/home/jkicinski/devel/linux/build_net-perf' GEN Makefile DESCEND objtool Using .. as source for kernel CALL ../scripts/atomic/check-atomics.sh CALL ../scripts/checksyscalls.sh CHK include/generated/compile.h CHECK ../lib/rhashtable.c ../lib/rhashtable.c:134:13: warning: incorrect type in initializer (different address spaces) ../lib/rhashtable.c:134:13: expected union nested_table [noderef] <asn:4> *__new ../lib/rhashtable.c:134:13: got union nested_table *[assigned] ntbl ../lib/rhashtable.c:299:13: warning: incorrect type in initializer (different address spaces) ../lib/rhashtable.c:299:13: expected struct bucket_table [noderef] <asn:4> *__new ../lib/rhashtable.c:299:13: got struct bucket_table *new_tbl [...]
On Wed, May 15, 2019 at 01:55:01PM -0700, Jakub Kicinski wrote: > Since the bit_spin_lock() operations don't actually dereference > the pointer, it's fine to forcefully drop the RCU annotation. > This fixes 7 sparse warnings per include site. > > Fixes: 8f0db018006a ("rhashtable: use bit_spin_locks to protect hash bucket.") > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > Reviewed-by: Simon Horman <simon.horman@netronome.com> I don't think this is the right fix. We should remove the __rcu marker from the opaque type rhash_lock_head since it cannot be directly dereferenced. I'm working on a fix to this. Thanks,
On Thu, 16 May 2019 13:16:23 +0800, Herbert Xu wrote: > On Wed, May 15, 2019 at 01:55:01PM -0700, Jakub Kicinski wrote: > > Since the bit_spin_lock() operations don't actually dereference > > the pointer, it's fine to forcefully drop the RCU annotation. > > This fixes 7 sparse warnings per include site. > > > > Fixes: 8f0db018006a ("rhashtable: use bit_spin_locks to protect hash bucket.") > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > > Reviewed-by: Simon Horman <simon.horman@netronome.com> > > I don't think this is the right fix. We should remove the __rcu > marker from the opaque type rhash_lock_head since it cannot be > directly dereferenced. > > I'm working on a fix to this. Make sense, anything that clears the warnings is fine with me :) Thanks!
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h index f7714d3b46bd..bea1e0440ab4 100644 --- a/include/linux/rhashtable.h +++ b/include/linux/rhashtable.h @@ -325,27 +325,27 @@ static inline struct rhash_lock_head __rcu **rht_bucket_insert( */ static inline void rht_lock(struct bucket_table *tbl, - struct rhash_lock_head **bkt) + struct rhash_lock_head __rcu **bkt) { local_bh_disable(); - bit_spin_lock(0, (unsigned long *)bkt); + bit_spin_lock(0, (unsigned long __force *)bkt); lock_map_acquire(&tbl->dep_map); } static inline void rht_lock_nested(struct bucket_table *tbl, - struct rhash_lock_head **bucket, + struct rhash_lock_head __rcu **bkt, unsigned int subclass) { local_bh_disable(); - bit_spin_lock(0, (unsigned long *)bucket); + bit_spin_lock(0, (unsigned long __force *)bkt); lock_acquire_exclusive(&tbl->dep_map, subclass, 0, NULL, _THIS_IP_); } static inline void rht_unlock(struct bucket_table *tbl, - struct rhash_lock_head **bkt) + struct rhash_lock_head __rcu **bkt) { lock_map_release(&tbl->dep_map); - bit_spin_unlock(0, (unsigned long *)bkt); + bit_spin_unlock(0, (unsigned long __force *)bkt); local_bh_enable(); }
Since the bit_spin_lock() operations don't actually dereference the pointer, it's fine to forcefully drop the RCU annotation. This fixes 7 sparse warnings per include site. Fixes: 8f0db018006a ("rhashtable: use bit_spin_locks to protect hash bucket.") Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> Reviewed-by: Simon Horman <simon.horman@netronome.com> --- include/linux/rhashtable.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)