[PATCH net] rhashtable: fix sparse RCU warnings on bit lock in bucket pointer

[PATCH net] rhashtable: fix sparse RCU warnings on bit lock in bucket pointer

[Jakub Kicinski]

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(-)

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

Re: [PATCH net] rhashtable: fix sparse RCU warnings on bit lock in bucket pointer

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH net] rhashtable: fix sparse RCU warnings on bit lock in bucket pointer

[Jakub Kicinski]

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
  [...]

Re: [PATCH net] rhashtable: fix sparse RCU warnings on bit lock in bucket pointer

[Herbert Xu]

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 0/2] rhashtable: Fix sparse warnings

[Herbert Xu]

This patch series fixes all the sparse warnings.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 1/2] rhashtable: Remove RCU marking from rhash_lock_head

[Herbert Xu]

The opaque type rhash_lock_head should not be marked with __rcu
because it can never be dereferenced.  We should apply the RCU
marking when we turn it into a pointer which can be dereferenced.

This patch does exactly that.  This fixes a number of sparse
warnings as well as getting rid of some unnecessary RCU checking.
   
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 include/linux/rhashtable.h |   58 ++++++++++++++++++++++++---------------------
 lib/rhashtable.c           |   28 ++++++++++-----------
 2 files changed, 46 insertions(+), 40 deletions(-)

diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index f7714d3b46bd..9f8bc06d4136 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -84,7 +84,7 @@ struct bucket_table {
 
 	struct lockdep_map	dep_map;
 
-	struct rhash_lock_head __rcu *buckets[] ____cacheline_aligned_in_smp;
+	struct rhash_lock_head *buckets[] ____cacheline_aligned_in_smp;
 };
 
 /*
@@ -261,13 +261,13 @@ void rhashtable_free_and_destroy(struct rhashtable *ht,
 				 void *arg);
 void rhashtable_destroy(struct rhashtable *ht);
 
-struct rhash_lock_head __rcu **rht_bucket_nested(const struct bucket_table *tbl,
-						 unsigned int hash);
-struct rhash_lock_head __rcu **__rht_bucket_nested(const struct bucket_table *tbl,
-						   unsigned int hash);
-struct rhash_lock_head __rcu **rht_bucket_nested_insert(struct rhashtable *ht,
-							struct bucket_table *tbl,
-							unsigned int hash);
+struct rhash_lock_head **rht_bucket_nested(const struct bucket_table *tbl,
+					   unsigned int hash);
+struct rhash_lock_head **__rht_bucket_nested(const struct bucket_table *tbl,
+					     unsigned int hash);
+struct rhash_lock_head **rht_bucket_nested_insert(struct rhashtable *ht,
+						  struct bucket_table *tbl,
+						  unsigned int hash);
 
 #define rht_dereference(p, ht) \
 	rcu_dereference_protected(p, lockdep_rht_mutex_is_held(ht))
@@ -284,21 +284,21 @@ struct rhash_lock_head __rcu **rht_bucket_nested_insert(struct rhashtable *ht,
 #define rht_entry(tpos, pos, member) \
 	({ tpos = container_of(pos, typeof(*tpos), member); 1; })
 
-static inline struct rhash_lock_head __rcu *const *rht_bucket(
+static inline struct rhash_lock_head *const *rht_bucket(
 	const struct bucket_table *tbl, unsigned int hash)
 {
 	return unlikely(tbl->nest) ? rht_bucket_nested(tbl, hash) :
 				     &tbl->buckets[hash];
 }
 
-static inline struct rhash_lock_head __rcu **rht_bucket_var(
+static inline struct rhash_lock_head **rht_bucket_var(
 	struct bucket_table *tbl, unsigned int hash)
 {
 	return unlikely(tbl->nest) ? __rht_bucket_nested(tbl, hash) :
 				     &tbl->buckets[hash];
 }
 
-static inline struct rhash_lock_head __rcu **rht_bucket_insert(
+static inline struct rhash_lock_head **rht_bucket_insert(
 	struct rhashtable *ht, struct bucket_table *tbl, unsigned int hash)
 {
 	return unlikely(tbl->nest) ? rht_bucket_nested_insert(ht, tbl, hash) :
@@ -349,6 +349,12 @@ static inline void rht_unlock(struct bucket_table *tbl,
 	local_bh_enable();
 }
 
+static inline struct rhash_head __rcu *__rht_ptr(
+	struct rhash_lock_head *const *bkt)
+{
+	return (struct rhash_head __rcu *)((unsigned long)*bkt & ~BIT(0));
+}
+
 /*
  * Where 'bkt' is a bucket and might be locked:
  *   rht_ptr() dereferences that pointer and clears the lock bit.
@@ -356,30 +362,30 @@ static inline void rht_unlock(struct bucket_table *tbl,
  *            access is guaranteed, such as when destroying the table.
  */
 static inline struct rhash_head *rht_ptr(
-	struct rhash_lock_head __rcu * const *bkt,
+	struct rhash_lock_head *const *bkt,
 	struct bucket_table *tbl,
 	unsigned int hash)
 {
-	const struct rhash_lock_head *p =
-		rht_dereference_bucket_rcu(*bkt, tbl, hash);
+	struct rhash_head __rcu *p = __rht_ptr(bkt);
 
-	if ((((unsigned long)p) & ~BIT(0)) == 0)
+	if (!p)
 		return RHT_NULLS_MARKER(bkt);
-	return (void *)(((unsigned long)p) & ~BIT(0));
+
+	return rht_dereference_bucket_rcu(p, tbl, hash);
 }
 
 static inline struct rhash_head *rht_ptr_exclusive(
-	struct rhash_lock_head __rcu * const *bkt)
+	struct rhash_lock_head *const *bkt)
 {
-	const struct rhash_lock_head *p =
-		rcu_dereference_protected(*bkt, 1);
+	struct rhash_head __rcu *p = __rht_ptr(bkt);
 
 	if (!p)
 		return RHT_NULLS_MARKER(bkt);
-	return (void *)(((unsigned long)p) & ~BIT(0));
+
+	return rcu_dereference_protected(p, 1);
 }
 
-static inline void rht_assign_locked(struct rhash_lock_head __rcu **bkt,
+static inline void rht_assign_locked(struct rhash_lock_head **bkt,
 				     struct rhash_head *obj)
 {
 	struct rhash_head __rcu **p = (struct rhash_head __rcu **)bkt;
@@ -390,7 +396,7 @@ static inline void rht_assign_locked(struct rhash_lock_head __rcu **bkt,
 }
 
 static inline void rht_assign_unlock(struct bucket_table *tbl,
-				     struct rhash_lock_head __rcu **bkt,
+				     struct rhash_lock_head **bkt,
 				     struct rhash_head *obj)
 {
 	struct rhash_head __rcu **p = (struct rhash_head __rcu **)bkt;
@@ -587,7 +593,7 @@ static inline struct rhash_head *__rhashtable_lookup(
 		.ht = ht,
 		.key = key,
 	};
-	struct rhash_lock_head __rcu * const *bkt;
+	struct rhash_lock_head *const *bkt;
 	struct bucket_table *tbl;
 	struct rhash_head *he;
 	unsigned int hash;
@@ -703,7 +709,7 @@ static inline void *__rhashtable_insert_fast(
 		.ht = ht,
 		.key = key,
 	};
-	struct rhash_lock_head __rcu **bkt;
+	struct rhash_lock_head **bkt;
 	struct rhash_head __rcu **pprev;
 	struct bucket_table *tbl;
 	struct rhash_head *head;
@@ -989,7 +995,7 @@ static inline int __rhashtable_remove_fast_one(
 	struct rhash_head *obj, const struct rhashtable_params params,
 	bool rhlist)
 {
-	struct rhash_lock_head __rcu **bkt;
+	struct rhash_lock_head **bkt;
 	struct rhash_head __rcu **pprev;
 	struct rhash_head *he;
 	unsigned int hash;
@@ -1141,7 +1147,7 @@ static inline int __rhashtable_replace_fast(
 	struct rhash_head *obj_old, struct rhash_head *obj_new,
 	const struct rhashtable_params params)
 {
-	struct rhash_lock_head __rcu **bkt;
+	struct rhash_lock_head **bkt;
 	struct rhash_head __rcu **pprev;
 	struct rhash_head *he;
 	unsigned int hash;
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 6529fe1b45c1..7708699a5b96 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -34,7 +34,7 @@
 
 union nested_table {
 	union nested_table __rcu *table;
-	struct rhash_lock_head __rcu *bucket;
+	struct rhash_lock_head *bucket;
 };
 
 static u32 head_hashfn(struct rhashtable *ht,
@@ -216,7 +216,7 @@ static struct bucket_table *rhashtable_last_table(struct rhashtable *ht,
 }
 
 static int rhashtable_rehash_one(struct rhashtable *ht,
-				 struct rhash_lock_head __rcu **bkt,
+				 struct rhash_lock_head **bkt,
 				 unsigned int old_hash)
 {
 	struct bucket_table *old_tbl = rht_dereference(ht->tbl, ht);
@@ -269,7 +269,7 @@ static int rhashtable_rehash_chain(struct rhashtable *ht,
 				    unsigned int old_hash)
 {
 	struct bucket_table *old_tbl = rht_dereference(ht->tbl, ht);
-	struct rhash_lock_head __rcu **bkt = rht_bucket_var(old_tbl, old_hash);
+	struct rhash_lock_head **bkt = rht_bucket_var(old_tbl, old_hash);
 	int err;
 
 	if (!bkt)
@@ -478,7 +478,7 @@ static int rhashtable_insert_rehash(struct rhashtable *ht,
 }
 
 static void *rhashtable_lookup_one(struct rhashtable *ht,
-				   struct rhash_lock_head __rcu **bkt,
+				   struct rhash_lock_head **bkt,
 				   struct bucket_table *tbl, unsigned int hash,
 				   const void *key, struct rhash_head *obj)
 {
@@ -529,7 +529,7 @@ static void *rhashtable_lookup_one(struct rhashtable *ht,
 }
 
 static struct bucket_table *rhashtable_insert_one(struct rhashtable *ht,
-						  struct rhash_lock_head __rcu **bkt,
+						  struct rhash_lock_head **bkt,
 						  struct bucket_table *tbl,
 						  unsigned int hash,
 						  struct rhash_head *obj,
@@ -584,7 +584,7 @@ static void *rhashtable_try_insert(struct rhashtable *ht, const void *key,
 {
 	struct bucket_table *new_tbl;
 	struct bucket_table *tbl;
-	struct rhash_lock_head __rcu **bkt;
+	struct rhash_lock_head **bkt;
 	unsigned int hash;
 	void *data;
 
@@ -1166,8 +1166,8 @@ void rhashtable_destroy(struct rhashtable *ht)
 }
 EXPORT_SYMBOL_GPL(rhashtable_destroy);
 
-struct rhash_lock_head __rcu **__rht_bucket_nested(const struct bucket_table *tbl,
-						   unsigned int hash)
+struct rhash_lock_head **__rht_bucket_nested(const struct bucket_table *tbl,
+					     unsigned int hash)
 {
 	const unsigned int shift = PAGE_SHIFT - ilog2(sizeof(void *));
 	unsigned int index = hash & ((1 << tbl->nest) - 1);
@@ -1195,10 +1195,10 @@ struct rhash_lock_head __rcu **__rht_bucket_nested(const struct bucket_table *tb
 }
 EXPORT_SYMBOL_GPL(__rht_bucket_nested);
 
-struct rhash_lock_head __rcu **rht_bucket_nested(const struct bucket_table *tbl,
-						 unsigned int hash)
+struct rhash_lock_head **rht_bucket_nested(const struct bucket_table *tbl,
+					   unsigned int hash)
 {
-	static struct rhash_lock_head __rcu *rhnull;
+	static struct rhash_lock_head *rhnull;
 
 	if (!rhnull)
 		INIT_RHT_NULLS_HEAD(rhnull);
@@ -1206,9 +1206,9 @@ struct rhash_lock_head __rcu **rht_bucket_nested(const struct bucket_table *tbl,
 }
 EXPORT_SYMBOL_GPL(rht_bucket_nested);
 
-struct rhash_lock_head __rcu **rht_bucket_nested_insert(struct rhashtable *ht,
-							struct bucket_table *tbl,
-							unsigned int hash)
+struct rhash_lock_head **rht_bucket_nested_insert(struct rhashtable *ht,
+						  struct bucket_table *tbl,
+						  unsigned int hash)
 {
 	const unsigned int shift = PAGE_SHIFT - ilog2(sizeof(void *));
 	unsigned int index = hash & ((1 << tbl->nest) - 1);

[PATCH 2/2] rhashtable: Fix cmpxchg RCU warnings

[Herbert Xu]

As cmpxchg is a non-RCU mechanism it will cause sparse warnings
when we use it for RCU.  This patch adds explicit casts to silence
those warnings.  This should probably be moved to RCU itself in
future.
  
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 lib/rhashtable.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 7708699a5b96..935ec80f213f 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -131,7 +131,7 @@ static union nested_table *nested_table_alloc(struct rhashtable *ht,
 			INIT_RHT_NULLS_HEAD(ntbl[i].bucket);
 	}
 
-	if (cmpxchg(prev, NULL, ntbl) == NULL)
+	if (cmpxchg((union nested_table **)prev, NULL, ntbl) == NULL)
 		return ntbl;
 	/* Raced with another thread. */
 	kfree(ntbl);
@@ -296,7 +296,8 @@ static int rhashtable_rehash_attach(struct rhashtable *ht,
 	 * rcu_assign_pointer().
 	 */
 
-	if (cmpxchg(&old_tbl->future_tbl, NULL, new_tbl) != NULL)
+	if (cmpxchg((struct bucket_table **)&old_tbl->future_tbl, NULL,
+		    new_tbl) != NULL)
 		return -EEXIST;
 
 	return 0;

RE: [PATCH 2/2] rhashtable: Fix cmpxchg RCU warnings

[David Laight]

From: Herbert Xu
> Sent: 16 May 2019 08:20
> As cmpxchg is a non-RCU mechanism it will cause sparse warnings
> when we use it for RCU.  This patch adds explicit casts to silence
> those warnings.  This should probably be moved to RCU itself in
> future.
> 
...
> -	if (cmpxchg(prev, NULL, ntbl) == NULL)
> +	if (cmpxchg((union nested_table **)prev, NULL, ntbl) == NULL)

I presume these casts remove an 'rcu' marker on the variable.
Is there a way of marking such casts as 'for sparse only' so
that the compiler does proper type checking.
(Clearly this isn't that relevant here as the cast could be (void **).)

Hmmm something should be checking that the type of the argument
to cmpxchg is 'pointer to "something the size of a pointer"'
Adding any kind of cast subverts that test.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 2/2] rhashtable: Fix cmpxchg RCU warnings

[Herbert Xu]

On Thu, May 16, 2019 at 09:20:36AM +0000, David Laight wrote:
>
> I presume these casts remove an 'rcu' marker on the variable.
> Is there a way of marking such casts as 'for sparse only' so
> that the compiler does proper type checking.
> (Clearly this isn't that relevant here as the cast could be (void **).)
> 
> Hmmm something should be checking that the type of the argument
> to cmpxchg is 'pointer to "something the size of a pointer"'
> Adding any kind of cast subverts that test.

If we were adding this as an RCU primitive then yes that what
it should do.  But it isn't relevant to this patch.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net] rhashtable: fix sparse RCU warnings on bit lock in bucket pointer

[Jakub Kicinski]

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!

Re: [PATCH 0/2] rhashtable: Fix sparse warnings

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 16 May 2019 15:18:59 +0800

> This patch series fixes all the sparse warnings.

Series applied, thanks Herbert.