linux-next: build warning after merge of the net-next tree

linux-next: build warning after merge of the net-next tree

[Stephen Rothwell]

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

Hi all,

After merging the net-next tree, today's linux-next build (powerpc
ppc64_defconfig) produced this warning:

net/bridge/br_multicast.c: In function 'br_multicast_find_port':
net/bridge/br_multicast.c:1818:21: warning: unused variable 'br' [-Wunused-variable]
 1818 |  struct net_bridge *br = mp->br;
      |                     ^~

Introduced by commit

  0436862e417e ("net: bridge: mcast: support for IGMPv3/MLDv2 ALLOW_NEW_SOURCES report")

Maybe turning mlock_dereference into a static inline function would help.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build warning after merge of the net-next tree

[Jakub Kicinski]

On Tue, 8 Sep 2020 13:00:00 +1000 Stephen Rothwell wrote:
> Hi all,
> 
> After merging the net-next tree, today's linux-next build (powerpc
> ppc64_defconfig) produced this warning:
> 
> net/bridge/br_multicast.c: In function 'br_multicast_find_port':
> net/bridge/br_multicast.c:1818:21: warning: unused variable 'br' [-Wunused-variable]
>  1818 |  struct net_bridge *br = mp->br;
>       |                     ^~
> 
> Introduced by commit
> 
>   0436862e417e ("net: bridge: mcast: support for IGMPv3/MLDv2 ALLOW_NEW_SOURCES report")
> 
> Maybe turning mlock_dereference into a static inline function would help.

Or perhaps provide a better definition of whatever is making the
reference disappear? RCU_LOCKDEP_WARN()?

Thanks for the report!

[PATCH net-next] net: bridge: mcast: fix unused br var when lockdep isn't defined

[Nikolay Aleksandrov]

Stephen reported the following warning:
 net/bridge/br_multicast.c: In function 'br_multicast_find_port':
 net/bridge/br_multicast.c:1818:21: warning: unused variable 'br' [-Wunused-variable]
  1818 |  struct net_bridge *br = mp->br;
       |                     ^~

It happens due to bridge's mlock_dereference() when lockdep isn't defined.
Silence the warning by annotating the variable as __maybe_unused.

Fixes: 0436862e417e ("net: bridge: mcast: support for IGMPv3/MLDv2 ALLOW_NEW_SOURCES report")
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_multicast.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index b83f11228948..33adf44ef7ec 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1814,8 +1814,8 @@ br_multicast_find_port(struct net_bridge_mdb_entry *mp,
 		       struct net_bridge_port *p,
 		       const unsigned char *src)
 {
+	struct net_bridge *br __maybe_unused = mp->br;
 	struct net_bridge_port_group *pg;
-	struct net_bridge *br = mp->br;
 
 	for (pg = mlock_dereference(mp->ports, br);
 	     pg;
-- 
2.25.4

Re: [PATCH net-next] net: bridge: mcast: fix unused br var when lockdep isn't defined

[Jakub Kicinski]

On Tue,  8 Sep 2020 10:17:13 +0300 Nikolay Aleksandrov wrote:
> Stephen reported the following warning:
>  net/bridge/br_multicast.c: In function 'br_multicast_find_port':
>  net/bridge/br_multicast.c:1818:21: warning: unused variable 'br' [-Wunused-variable]
>   1818 |  struct net_bridge *br = mp->br;
>        |                     ^~
> 
> It happens due to bridge's mlock_dereference() when lockdep isn't defined.
> Silence the warning by annotating the variable as __maybe_unused.
> 
> Fixes: 0436862e417e ("net: bridge: mcast: support for IGMPv3/MLDv2 ALLOW_NEW_SOURCES report")
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
>  net/bridge/br_multicast.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index b83f11228948..33adf44ef7ec 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1814,8 +1814,8 @@ br_multicast_find_port(struct net_bridge_mdb_entry *mp,
>  		       struct net_bridge_port *p,
>  		       const unsigned char *src)
>  {
> +	struct net_bridge *br __maybe_unused = mp->br;
>  	struct net_bridge_port_group *pg;
> -	struct net_bridge *br = mp->br;
>  
>  	for (pg = mlock_dereference(mp->ports, br);
>  	     pg;

That's a lazy fix :( Is everyone using lockdep annotations going to
sprinkle __maybe_unused throughout the code? Macros should also always
evaluate their arguments.

Re: [PATCH net-next] net: bridge: mcast: fix unused br var when lockdep isn't defined

[nikolay]

On 8 September 2020 19:00:49 EEST, Jakub Kicinski <kuba@kernel.org> wrote:
>On Tue,  8 Sep 2020 10:17:13 +0300 Nikolay Aleksandrov wrote:
>> Stephen reported the following warning:
>>  net/bridge/br_multicast.c: In function 'br_multicast_find_port':
>>  net/bridge/br_multicast.c:1818:21: warning: unused variable 'br'
>[-Wunused-variable]
>>   1818 |  struct net_bridge *br = mp->br;
>>        |                     ^~
>> 
>> It happens due to bridge's mlock_dereference() when lockdep isn't
>defined.
>> Silence the warning by annotating the variable as __maybe_unused.
>> 
>> Fixes: 0436862e417e ("net: bridge: mcast: support for IGMPv3/MLDv2
>ALLOW_NEW_SOURCES report")
>> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> ---
>>  net/bridge/br_multicast.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index b83f11228948..33adf44ef7ec 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -1814,8 +1814,8 @@ br_multicast_find_port(struct
>net_bridge_mdb_entry *mp,
>>  		       struct net_bridge_port *p,
>>  		       const unsigned char *src)
>>  {
>> +	struct net_bridge *br __maybe_unused = mp->br;
>>  	struct net_bridge_port_group *pg;
>> -	struct net_bridge *br = mp->br;
>>  
>>  	for (pg = mlock_dereference(mp->ports, br);
>>  	     pg;
>
>That's a lazy fix :( Is everyone using lockdep annotations going to
>sprinkle __maybe_unused throughout the code? Macros should also always
>evaluate their arguments.

When the local variable's only used for lockdep, I guess. :) 
Here we don't actually need it at all, alternatively we can just drop it and use mp->br. 

[PATCH net-next] rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition

[Jakub Kicinski]

We run into a unused variable warning in bridge code when
variable is only used inside the condition of
rcu_dereference_protected().

 #define mlock_dereference(X, br) \
	rcu_dereference_protected(X, lockdep_is_held(&br->multicast_lock))

Since on builds with CONFIG_PROVE_RCU=n rcu_dereference_protected()
compiles to nothing the compiler doesn't see the variable use.

Prevent the warning by adding the condition as dead code.
We need to un-hide the declaration of lockdep_tasklist_lock_is_held()
and fix a bug the crept into a net/sched header.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/rcupdate.h   | 2 +-
 include/linux/sched/task.h | 2 --
 include/net/sch_generic.h  | 2 +-
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index d15d46db61f7..cf3d3ba3f3e4 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -320,7 +320,7 @@ static inline void rcu_preempt_sleep_check(void) { }
 
 #else /* #ifdef CONFIG_PROVE_RCU */
 
-#define RCU_LOCKDEP_WARN(c, s) do { } while (0)
+#define RCU_LOCKDEP_WARN(c, s) do { } while (0 && (c))
 #define rcu_sleep_check() do { } while (0)
 
 #endif /* #else #ifdef CONFIG_PROVE_RCU */
diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index a98965007eef..9f943c391df9 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -47,9 +47,7 @@ extern spinlock_t mmlist_lock;
 extern union thread_union init_thread_union;
 extern struct task_struct init_task;
 
-#ifdef CONFIG_PROVE_RCU
 extern int lockdep_tasklist_lock_is_held(void);
-#endif /* #ifdef CONFIG_PROVE_RCU */
 
 extern asmlinkage void schedule_tail(struct task_struct *prev);
 extern void init_idle(struct task_struct *idle, int cpu);
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index d60e7c39d60c..eb68cc6e4e79 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -443,7 +443,7 @@ static inline bool lockdep_tcf_proto_is_locked(struct tcf_proto *tp)
 	return lockdep_is_held(&tp->lock);
 }
 #else
-static inline bool lockdep_tcf_chain_is_locked(struct tcf_block *chain)
+static inline bool lockdep_tcf_chain_is_locked(struct tcf_chain *chain)
 {
 	return true;
 }
-- 
2.24.1

Re: [PATCH net-next] rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition

[nikolay]

On 8 September 2020 20:36:24 EEST, Jakub Kicinski <kuba@kernel.org> wrote:
>We run into a unused variable warning in bridge code when
>variable is only used inside the condition of
>rcu_dereference_protected().
>
> #define mlock_dereference(X, br) \
>	rcu_dereference_protected(X, lockdep_is_held(&br->multicast_lock))
>
>Since on builds with CONFIG_PROVE_RCU=n rcu_dereference_protected()
>compiles to nothing the compiler doesn't see the variable use.
>
>Prevent the warning by adding the condition as dead code.
>We need to un-hide the declaration of lockdep_tasklist_lock_is_held()
>and fix a bug the crept into a net/sched header.
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>---
> include/linux/rcupdate.h   | 2 +-
> include/linux/sched/task.h | 2 --
> include/net/sch_generic.h  | 2 +-
> 3 files changed, 2 insertions(+), 4 deletions(-)
>
>diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
>index d15d46db61f7..cf3d3ba3f3e4 100644
>--- a/include/linux/rcupdate.h
>+++ b/include/linux/rcupdate.h
>@@ -320,7 +320,7 @@ static inline void rcu_preempt_sleep_check(void) {
>}
> 
> #else /* #ifdef CONFIG_PROVE_RCU */
> 
>-#define RCU_LOCKDEP_WARN(c, s) do { } while (0)
>+#define RCU_LOCKDEP_WARN(c, s) do { } while (0 && (c))
> #define rcu_sleep_check() do { } while (0)
> 
> #endif /* #else #ifdef CONFIG_PROVE_RCU */
>diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
>index a98965007eef..9f943c391df9 100644
>--- a/include/linux/sched/task.h
>+++ b/include/linux/sched/task.h
>@@ -47,9 +47,7 @@ extern spinlock_t mmlist_lock;
> extern union thread_union init_thread_union;
> extern struct task_struct init_task;
> 
>-#ifdef CONFIG_PROVE_RCU
> extern int lockdep_tasklist_lock_is_held(void);
>-#endif /* #ifdef CONFIG_PROVE_RCU */
> 
> extern asmlinkage void schedule_tail(struct task_struct *prev);
> extern void init_idle(struct task_struct *idle, int cpu);
>diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
>index d60e7c39d60c..eb68cc6e4e79 100644
>--- a/include/net/sch_generic.h
>+++ b/include/net/sch_generic.h
>@@ -443,7 +443,7 @@ static inline bool
>lockdep_tcf_proto_is_locked(struct tcf_proto *tp)
> 	return lockdep_is_held(&tp->lock);
> }
> #else
>-static inline bool lockdep_tcf_chain_is_locked(struct tcf_block
>*chain)
>+static inline bool lockdep_tcf_chain_is_locked(struct tcf_chain
>*chain)
> {
> 	return true;
> }

Ah, you want to solve it for all. :) 
Looks and sounds good to me, 
Reviewed-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Re: [PATCH net-next] rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition

[Jakub Kicinski]

On Tue, 08 Sep 2020 21:15:56 +0300 nikolay@cumulusnetworks.com wrote:
> Ah, you want to solve it for all. :) 
> Looks and sounds good to me, 
> Reviewed-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

Actually, I give up, lockdep_is_held() is not defined without
CONFIG_LOCKDEP, let's just go with your patch..

Re: [PATCH net-next] rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition

[David Miller]

From: Jakub Kicinski <kuba@kernel.org>
Date: Tue,  8 Sep 2020 10:36:24 -0700

> We run into a unused variable warning in bridge code when
> variable is only used inside the condition of
> rcu_dereference_protected().
> 
>  #define mlock_dereference(X, br) \
> 	rcu_dereference_protected(X, lockdep_is_held(&br->multicast_lock))
> 
> Since on builds with CONFIG_PROVE_RCU=n rcu_dereference_protected()
> compiles to nothing the compiler doesn't see the variable use.
> 
> Prevent the warning by adding the condition as dead code.
> We need to un-hide the declaration of lockdep_tasklist_lock_is_held()
> and fix a bug the crept into a net/sched header.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

I ended up applying Nikolay's fix, but this situation with the rcu macros
needs to be addressed.

Re: [PATCH net-next] rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition

[Joel Fernandes]

On Tue, Sep 08, 2020 at 05:27:51PM -0700, Jakub Kicinski wrote:
> On Tue, 08 Sep 2020 21:15:56 +0300 nikolay@cumulusnetworks.com wrote:
> > Ah, you want to solve it for all. :) 
> > Looks and sounds good to me, 
> > Reviewed-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> 
> Actually, I give up, lockdep_is_held() is not defined without
> CONFIG_LOCKDEP, let's just go with your patch..

Care to send a patch just for the RCU macro then? Not sure what Dave is
applying but if the net-next tree is not taking the RCU macro change, then
send another one with my tag:

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks!

 - Joel

Re: [PATCH net-next] rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition

[Jakub Kicinski]

On Mon, 14 Sep 2020 16:21:22 -0400 Joel Fernandes wrote:
> On Tue, Sep 08, 2020 at 05:27:51PM -0700, Jakub Kicinski wrote:
> > On Tue, 08 Sep 2020 21:15:56 +0300 nikolay@cumulusnetworks.com wrote:  
> > > Ah, you want to solve it for all. :) 
> > > Looks and sounds good to me, 
> > > Reviewed-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>  
> > 
> > Actually, I give up, lockdep_is_held() is not defined without
> > CONFIG_LOCKDEP, let's just go with your patch..  
> 
> Care to send a patch just for the RCU macro then? Not sure what Dave is
> applying but if the net-next tree is not taking the RCU macro change, then
> send another one with my tag:

Seems like quite a few places depend on the macro disappearing its
argument. I was concerned that it's going to be had to pick out whether
!LOCKDEP builds should return true or false from LOCKDEP helpers, but
perhaps relying on the linker errors even more is not such poor taste?

Does the patch below look acceptable to you?

--->8------------

rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition

We run into a unused variable warning in bridge code when
variable is only used inside the condition of
rcu_dereference_protected().

 #define mlock_dereference(X, br) \
	rcu_dereference_protected(X, lockdep_is_held(&br->multicast_lock))

Since on builds with CONFIG_PROVE_RCU=n rcu_dereference_protected()
compiles to nothing the compiler doesn't see the variable use.

Prevent the warning by adding the condition as dead code.
We need to un-hide the declaration of lockdep_tasklist_lock_is_held(),
lockdep_sock_is_held(), RCU lock maps and remove some declarations
in net/sched header, because they have a wrong type.

Add forward declarations of lockdep_is_held(), lock_is_held() which
will cause a linker errors if actually used with !LOCKDEP.
At least RCU expects some locks _not_ to be held so it's hard to
pick true/false for a dummy implementation.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/lockdep.h        |  6 ++++++
 include/linux/rcupdate.h       | 11 ++++++-----
 include/linux/rcupdate_trace.h |  4 ++--
 include/linux/sched/task.h     |  2 --
 include/net/sch_generic.h      | 12 ------------
 include/net/sock.h             |  2 --
 6 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 6a584b3e5c74..c4b6225ee320 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -371,6 +371,12 @@ static inline void lockdep_unregister_key(struct lock_class_key *key)
 
 #define lockdep_depth(tsk)	(0)
 
+/*
+ * Dummy forward declarations, allow users to write less ifdef-y code
+ * and depend on dead code elimination.
+ */
+int lock_is_held(const void *);
+int lockdep_is_held(const void *);
 #define lockdep_is_held_type(l, r)		(1)
 
 #define lockdep_assert_held(l)			do { (void)(l); } while (0)
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index d15d46db61f7..50d45781fa99 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -234,6 +234,11 @@ bool rcu_lockdep_current_cpu_online(void);
 static inline bool rcu_lockdep_current_cpu_online(void) { return true; }
 #endif /* #else #if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PROVE_RCU) */
 
+extern struct lockdep_map rcu_lock_map;
+extern struct lockdep_map rcu_bh_lock_map;
+extern struct lockdep_map rcu_sched_lock_map;
+extern struct lockdep_map rcu_callback_map;
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 
 static inline void rcu_lock_acquire(struct lockdep_map *map)
@@ -246,10 +251,6 @@ static inline void rcu_lock_release(struct lockdep_map *map)
 	lock_release(map, _THIS_IP_);
 }
 
-extern struct lockdep_map rcu_lock_map;
-extern struct lockdep_map rcu_bh_lock_map;
-extern struct lockdep_map rcu_sched_lock_map;
-extern struct lockdep_map rcu_callback_map;
 int debug_lockdep_rcu_enabled(void);
 int rcu_read_lock_held(void);
 int rcu_read_lock_bh_held(void);
@@ -320,7 +321,7 @@ static inline void rcu_preempt_sleep_check(void) { }
 
 #else /* #ifdef CONFIG_PROVE_RCU */
 
-#define RCU_LOCKDEP_WARN(c, s) do { } while (0)
+#define RCU_LOCKDEP_WARN(c, s) do { } while (0 && (c))
 #define rcu_sleep_check() do { } while (0)
 
 #endif /* #else #ifdef CONFIG_PROVE_RCU */
diff --git a/include/linux/rcupdate_trace.h b/include/linux/rcupdate_trace.h
index aaaac8ac927c..25cdef506cae 100644
--- a/include/linux/rcupdate_trace.h
+++ b/include/linux/rcupdate_trace.h
@@ -11,10 +11,10 @@
 #include <linux/sched.h>
 #include <linux/rcupdate.h>
 
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-
 extern struct lockdep_map rcu_trace_lock_map;
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+
 static inline int rcu_read_lock_trace_held(void)
 {
 	return lock_is_held(&rcu_trace_lock_map);
diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index a98965007eef..9f943c391df9 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -47,9 +47,7 @@ extern spinlock_t mmlist_lock;
 extern union thread_union init_thread_union;
 extern struct task_struct init_task;
 
-#ifdef CONFIG_PROVE_RCU
 extern int lockdep_tasklist_lock_is_held(void);
-#endif /* #ifdef CONFIG_PROVE_RCU */
 
 extern asmlinkage void schedule_tail(struct task_struct *prev);
 extern void init_idle(struct task_struct *idle, int cpu);
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index d60e7c39d60c..1aaa9e3d2e9c 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -432,7 +432,6 @@ struct tcf_block {
 	struct mutex proto_destroy_lock; /* Lock for proto_destroy hashtable. */
 };
 
-#ifdef CONFIG_PROVE_LOCKING
 static inline bool lockdep_tcf_chain_is_locked(struct tcf_chain *chain)
 {
 	return lockdep_is_held(&chain->filter_chain_lock);
@@ -442,17 +441,6 @@ static inline bool lockdep_tcf_proto_is_locked(struct tcf_proto *tp)
 {
 	return lockdep_is_held(&tp->lock);
 }
-#else
-static inline bool lockdep_tcf_chain_is_locked(struct tcf_block *chain)
-{
-	return true;
-}
-
-static inline bool lockdep_tcf_proto_is_locked(struct tcf_proto *tp)
-{
-	return true;
-}
-#endif /* #ifdef CONFIG_PROVE_LOCKING */
 
 #define tcf_chain_dereference(p, chain)					\
 	rcu_dereference_protected(p, lockdep_tcf_chain_is_locked(chain))
diff --git a/include/net/sock.h b/include/net/sock.h
index eaa5cac5e836..1c67b1297a72 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1566,13 +1566,11 @@ do {									\
 	lockdep_init_map(&(sk)->sk_lock.dep_map, (name), (key), 0);	\
 } while (0)
 
-#ifdef CONFIG_LOCKDEP
 static inline bool lockdep_sock_is_held(const struct sock *sk)
 {
 	return lockdep_is_held(&sk->sk_lock) ||
 	       lockdep_is_held(&sk->sk_lock.slock);
 }
-#endif
 
 void lock_sock_nested(struct sock *sk, int subclass);
 
-- 
2.24.1

Re: [PATCH net-next] rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition

[Paul E. McKenney]

On Mon, Sep 14, 2020 at 03:47:38PM -0700, Jakub Kicinski wrote:
> On Mon, 14 Sep 2020 16:21:22 -0400 Joel Fernandes wrote:
> > On Tue, Sep 08, 2020 at 05:27:51PM -0700, Jakub Kicinski wrote:
> > > On Tue, 08 Sep 2020 21:15:56 +0300 nikolay@cumulusnetworks.com wrote:  
> > > > Ah, you want to solve it for all. :) 
> > > > Looks and sounds good to me, 
> > > > Reviewed-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>  
> > > 
> > > Actually, I give up, lockdep_is_held() is not defined without
> > > CONFIG_LOCKDEP, let's just go with your patch..  
> > 
> > Care to send a patch just for the RCU macro then? Not sure what Dave is
> > applying but if the net-next tree is not taking the RCU macro change, then
> > send another one with my tag:
> 
> Seems like quite a few places depend on the macro disappearing its
> argument. I was concerned that it's going to be had to pick out whether
> !LOCKDEP builds should return true or false from LOCKDEP helpers, but
> perhaps relying on the linker errors even more is not such poor taste?
> 
> Does the patch below look acceptable to you?

The thing to check would be whether all compilers do sufficient
dead-code elimination (it used to be that they did not).  One way to
get a quick sniff test of this would be to make sure that a dead-code
lockdep_is_held() is in common code, and then expose this patch to kbuild
test robot.

Seem reasonable?

							Thanx, Paul

> --->8------------
> 
> rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition
> 
> We run into a unused variable warning in bridge code when
> variable is only used inside the condition of
> rcu_dereference_protected().
> 
>  #define mlock_dereference(X, br) \
> 	rcu_dereference_protected(X, lockdep_is_held(&br->multicast_lock))
> 
> Since on builds with CONFIG_PROVE_RCU=n rcu_dereference_protected()
> compiles to nothing the compiler doesn't see the variable use.
> 
> Prevent the warning by adding the condition as dead code.
> We need to un-hide the declaration of lockdep_tasklist_lock_is_held(),
> lockdep_sock_is_held(), RCU lock maps and remove some declarations
> in net/sched header, because they have a wrong type.
> 
> Add forward declarations of lockdep_is_held(), lock_is_held() which
> will cause a linker errors if actually used with !LOCKDEP.
> At least RCU expects some locks _not_ to be held so it's hard to
> pick true/false for a dummy implementation.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  include/linux/lockdep.h        |  6 ++++++
>  include/linux/rcupdate.h       | 11 ++++++-----
>  include/linux/rcupdate_trace.h |  4 ++--
>  include/linux/sched/task.h     |  2 --
>  include/net/sch_generic.h      | 12 ------------
>  include/net/sock.h             |  2 --
>  6 files changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 6a584b3e5c74..c4b6225ee320 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -371,6 +371,12 @@ static inline void lockdep_unregister_key(struct lock_class_key *key)
>  
>  #define lockdep_depth(tsk)	(0)
>  
> +/*
> + * Dummy forward declarations, allow users to write less ifdef-y code
> + * and depend on dead code elimination.
> + */
> +int lock_is_held(const void *);
> +int lockdep_is_held(const void *);
>  #define lockdep_is_held_type(l, r)		(1)
>  
>  #define lockdep_assert_held(l)			do { (void)(l); } while (0)
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index d15d46db61f7..50d45781fa99 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -234,6 +234,11 @@ bool rcu_lockdep_current_cpu_online(void);
>  static inline bool rcu_lockdep_current_cpu_online(void) { return true; }
>  #endif /* #else #if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PROVE_RCU) */
>  
> +extern struct lockdep_map rcu_lock_map;
> +extern struct lockdep_map rcu_bh_lock_map;
> +extern struct lockdep_map rcu_sched_lock_map;
> +extern struct lockdep_map rcu_callback_map;
> +
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>  
>  static inline void rcu_lock_acquire(struct lockdep_map *map)
> @@ -246,10 +251,6 @@ static inline void rcu_lock_release(struct lockdep_map *map)
>  	lock_release(map, _THIS_IP_);
>  }
>  
> -extern struct lockdep_map rcu_lock_map;
> -extern struct lockdep_map rcu_bh_lock_map;
> -extern struct lockdep_map rcu_sched_lock_map;
> -extern struct lockdep_map rcu_callback_map;
>  int debug_lockdep_rcu_enabled(void);
>  int rcu_read_lock_held(void);
>  int rcu_read_lock_bh_held(void);
> @@ -320,7 +321,7 @@ static inline void rcu_preempt_sleep_check(void) { }
>  
>  #else /* #ifdef CONFIG_PROVE_RCU */
>  
> -#define RCU_LOCKDEP_WARN(c, s) do { } while (0)
> +#define RCU_LOCKDEP_WARN(c, s) do { } while (0 && (c))
>  #define rcu_sleep_check() do { } while (0)
>  
>  #endif /* #else #ifdef CONFIG_PROVE_RCU */
> diff --git a/include/linux/rcupdate_trace.h b/include/linux/rcupdate_trace.h
> index aaaac8ac927c..25cdef506cae 100644
> --- a/include/linux/rcupdate_trace.h
> +++ b/include/linux/rcupdate_trace.h
> @@ -11,10 +11,10 @@
>  #include <linux/sched.h>
>  #include <linux/rcupdate.h>
>  
> -#ifdef CONFIG_DEBUG_LOCK_ALLOC
> -
>  extern struct lockdep_map rcu_trace_lock_map;
>  
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +
>  static inline int rcu_read_lock_trace_held(void)
>  {
>  	return lock_is_held(&rcu_trace_lock_map);
> diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
> index a98965007eef..9f943c391df9 100644
> --- a/include/linux/sched/task.h
> +++ b/include/linux/sched/task.h
> @@ -47,9 +47,7 @@ extern spinlock_t mmlist_lock;
>  extern union thread_union init_thread_union;
>  extern struct task_struct init_task;
>  
> -#ifdef CONFIG_PROVE_RCU
>  extern int lockdep_tasklist_lock_is_held(void);
> -#endif /* #ifdef CONFIG_PROVE_RCU */
>  
>  extern asmlinkage void schedule_tail(struct task_struct *prev);
>  extern void init_idle(struct task_struct *idle, int cpu);
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index d60e7c39d60c..1aaa9e3d2e9c 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -432,7 +432,6 @@ struct tcf_block {
>  	struct mutex proto_destroy_lock; /* Lock for proto_destroy hashtable. */
>  };
>  
> -#ifdef CONFIG_PROVE_LOCKING
>  static inline bool lockdep_tcf_chain_is_locked(struct tcf_chain *chain)
>  {
>  	return lockdep_is_held(&chain->filter_chain_lock);
> @@ -442,17 +441,6 @@ static inline bool lockdep_tcf_proto_is_locked(struct tcf_proto *tp)
>  {
>  	return lockdep_is_held(&tp->lock);
>  }
> -#else
> -static inline bool lockdep_tcf_chain_is_locked(struct tcf_block *chain)
> -{
> -	return true;
> -}
> -
> -static inline bool lockdep_tcf_proto_is_locked(struct tcf_proto *tp)
> -{
> -	return true;
> -}
> -#endif /* #ifdef CONFIG_PROVE_LOCKING */
>  
>  #define tcf_chain_dereference(p, chain)					\
>  	rcu_dereference_protected(p, lockdep_tcf_chain_is_locked(chain))
> diff --git a/include/net/sock.h b/include/net/sock.h
> index eaa5cac5e836..1c67b1297a72 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1566,13 +1566,11 @@ do {									\
>  	lockdep_init_map(&(sk)->sk_lock.dep_map, (name), (key), 0);	\
>  } while (0)
>  
> -#ifdef CONFIG_LOCKDEP
>  static inline bool lockdep_sock_is_held(const struct sock *sk)
>  {
>  	return lockdep_is_held(&sk->sk_lock) ||
>  	       lockdep_is_held(&sk->sk_lock.slock);
>  }
> -#endif
>  
>  void lock_sock_nested(struct sock *sk, int subclass);
>  
> -- 
> 2.24.1
> 

Re: [PATCH net-next] rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition

[Jakub Kicinski]

On Mon, 14 Sep 2020 17:20:11 -0700 Paul E. McKenney wrote:
> > Seems like quite a few places depend on the macro disappearing its
> > argument. I was concerned that it's going to be had to pick out whether
> > !LOCKDEP builds should return true or false from LOCKDEP helpers, but
> > perhaps relying on the linker errors even more is not such poor taste?
> > 
> > Does the patch below look acceptable to you?  
> 
> The thing to check would be whether all compilers do sufficient
> dead-code elimination (it used to be that they did not).  One way to
> get a quick sniff test of this would be to make sure that a dead-code
> lockdep_is_held() is in common code, and then expose this patch to kbuild
> test robot.

I'm pretty sure it's in common code because kbuild bot complaints were
the reason I gave up the first time around ;) 

I'll expose this to kbuild bot via my kernel.org tree in case it
doesn't consider scissored patches and report back!

Re: [PATCH net-next] rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition

[Joel Fernandes]

On Mon, Sep 14, 2020 at 03:47:38PM -0700, Jakub Kicinski wrote:
> On Mon, 14 Sep 2020 16:21:22 -0400 Joel Fernandes wrote:
> > On Tue, Sep 08, 2020 at 05:27:51PM -0700, Jakub Kicinski wrote:
> > > On Tue, 08 Sep 2020 21:15:56 +0300 nikolay@cumulusnetworks.com wrote:  
> > > > Ah, you want to solve it for all. :) 
> > > > Looks and sounds good to me, 
> > > > Reviewed-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>  
> > > 
> > > Actually, I give up, lockdep_is_held() is not defined without
> > > CONFIG_LOCKDEP, let's just go with your patch..  
> > 
> > Care to send a patch just for the RCU macro then? Not sure what Dave is
> > applying but if the net-next tree is not taking the RCU macro change, then
> > send another one with my tag:
> 
> Seems like quite a few places depend on the macro disappearing its
> argument. I was concerned that it's going to be had to pick out whether
> !LOCKDEP builds should return true or false from LOCKDEP helpers, but
> perhaps relying on the linker errors even more is not such poor taste?
> 
> Does the patch below look acceptable to you?
> 
> --->8------------
> 
> rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition
> 
> We run into a unused variable warning in bridge code when
> variable is only used inside the condition of
> rcu_dereference_protected().
> 
>  #define mlock_dereference(X, br) \
> 	rcu_dereference_protected(X, lockdep_is_held(&br->multicast_lock))
> 
> Since on builds with CONFIG_PROVE_RCU=n rcu_dereference_protected()
> compiles to nothing the compiler doesn't see the variable use.
> 
> Prevent the warning by adding the condition as dead code.
> We need to un-hide the declaration of lockdep_tasklist_lock_is_held(),
> lockdep_sock_is_held(), RCU lock maps and remove some declarations
> in net/sched header, because they have a wrong type.
> 
> Add forward declarations of lockdep_is_held(), lock_is_held() which
> will cause a linker errors if actually used with !LOCKDEP.
> At least RCU expects some locks _not_ to be held so it's hard to
> pick true/false for a dummy implementation.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  include/linux/lockdep.h        |  6 ++++++
>  include/linux/rcupdate.h       | 11 ++++++-----
>  include/linux/rcupdate_trace.h |  4 ++--
>  include/linux/sched/task.h     |  2 --
>  include/net/sch_generic.h      | 12 ------------
>  include/net/sock.h             |  2 --

Would it make sense to split it into individual patches?

So 1 for rcu, 1 for lockdep and then 1 for networking. The lockdep ones may
need PeterZ's ack.

thanks,

 - Joel


>  6 files changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 6a584b3e5c74..c4b6225ee320 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -371,6 +371,12 @@ static inline void lockdep_unregister_key(struct lock_class_key *key)
>  
>  #define lockdep_depth(tsk)	(0)
>  
> +/*
> + * Dummy forward declarations, allow users to write less ifdef-y code
> + * and depend on dead code elimination.
> + */
> +int lock_is_held(const void *);
> +int lockdep_is_held(const void *);
>  #define lockdep_is_held_type(l, r)		(1)
>  
>  #define lockdep_assert_held(l)			do { (void)(l); } while (0)
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index d15d46db61f7..50d45781fa99 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -234,6 +234,11 @@ bool rcu_lockdep_current_cpu_online(void);
>  static inline bool rcu_lockdep_current_cpu_online(void) { return true; }
>  #endif /* #else #if defined(CONFIG_HOTPLUG_CPU) && defined(CONFIG_PROVE_RCU) */
>  
> +extern struct lockdep_map rcu_lock_map;
> +extern struct lockdep_map rcu_bh_lock_map;
> +extern struct lockdep_map rcu_sched_lock_map;
> +extern struct lockdep_map rcu_callback_map;
> +
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>  
>  static inline void rcu_lock_acquire(struct lockdep_map *map)
> @@ -246,10 +251,6 @@ static inline void rcu_lock_release(struct lockdep_map *map)
>  	lock_release(map, _THIS_IP_);
>  }
>  
> -extern struct lockdep_map rcu_lock_map;
> -extern struct lockdep_map rcu_bh_lock_map;
> -extern struct lockdep_map rcu_sched_lock_map;
> -extern struct lockdep_map rcu_callback_map;
>  int debug_lockdep_rcu_enabled(void);
>  int rcu_read_lock_held(void);
>  int rcu_read_lock_bh_held(void);
> @@ -320,7 +321,7 @@ static inline void rcu_preempt_sleep_check(void) { }
>  
>  #else /* #ifdef CONFIG_PROVE_RCU */
>  
> -#define RCU_LOCKDEP_WARN(c, s) do { } while (0)
> +#define RCU_LOCKDEP_WARN(c, s) do { } while (0 && (c))
>  #define rcu_sleep_check() do { } while (0)
>  
>  #endif /* #else #ifdef CONFIG_PROVE_RCU */
> diff --git a/include/linux/rcupdate_trace.h b/include/linux/rcupdate_trace.h
> index aaaac8ac927c..25cdef506cae 100644
> --- a/include/linux/rcupdate_trace.h
> +++ b/include/linux/rcupdate_trace.h
> @@ -11,10 +11,10 @@
>  #include <linux/sched.h>
>  #include <linux/rcupdate.h>
>  
> -#ifdef CONFIG_DEBUG_LOCK_ALLOC
> -
>  extern struct lockdep_map rcu_trace_lock_map;
>  
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +
>  static inline int rcu_read_lock_trace_held(void)
>  {
>  	return lock_is_held(&rcu_trace_lock_map);
> diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
> index a98965007eef..9f943c391df9 100644
> --- a/include/linux/sched/task.h
> +++ b/include/linux/sched/task.h
> @@ -47,9 +47,7 @@ extern spinlock_t mmlist_lock;
>  extern union thread_union init_thread_union;
>  extern struct task_struct init_task;
>  
> -#ifdef CONFIG_PROVE_RCU
>  extern int lockdep_tasklist_lock_is_held(void);
> -#endif /* #ifdef CONFIG_PROVE_RCU */
>  
>  extern asmlinkage void schedule_tail(struct task_struct *prev);
>  extern void init_idle(struct task_struct *idle, int cpu);
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index d60e7c39d60c..1aaa9e3d2e9c 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -432,7 +432,6 @@ struct tcf_block {
>  	struct mutex proto_destroy_lock; /* Lock for proto_destroy hashtable. */
>  };
>  
> -#ifdef CONFIG_PROVE_LOCKING
>  static inline bool lockdep_tcf_chain_is_locked(struct tcf_chain *chain)
>  {
>  	return lockdep_is_held(&chain->filter_chain_lock);
> @@ -442,17 +441,6 @@ static inline bool lockdep_tcf_proto_is_locked(struct tcf_proto *tp)
>  {
>  	return lockdep_is_held(&tp->lock);
>  }
> -#else
> -static inline bool lockdep_tcf_chain_is_locked(struct tcf_block *chain)
> -{
> -	return true;
> -}
> -
> -static inline bool lockdep_tcf_proto_is_locked(struct tcf_proto *tp)
> -{
> -	return true;
> -}
> -#endif /* #ifdef CONFIG_PROVE_LOCKING */
>  
>  #define tcf_chain_dereference(p, chain)					\
>  	rcu_dereference_protected(p, lockdep_tcf_chain_is_locked(chain))
> diff --git a/include/net/sock.h b/include/net/sock.h
> index eaa5cac5e836..1c67b1297a72 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1566,13 +1566,11 @@ do {									\
>  	lockdep_init_map(&(sk)->sk_lock.dep_map, (name), (key), 0);	\
>  } while (0)
>  
> -#ifdef CONFIG_LOCKDEP
>  static inline bool lockdep_sock_is_held(const struct sock *sk)
>  {
>  	return lockdep_is_held(&sk->sk_lock) ||
>  	       lockdep_is_held(&sk->sk_lock.slock);
>  }
> -#endif
>  
>  void lock_sock_nested(struct sock *sk, int subclass);
>  
> -- 
> 2.24.1
> 

Re: [PATCH net-next] rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition

[Paul E. McKenney]

On Mon, Sep 14, 2020 at 05:30:29PM -0700, Jakub Kicinski wrote:
> On Mon, 14 Sep 2020 17:20:11 -0700 Paul E. McKenney wrote:
> > > Seems like quite a few places depend on the macro disappearing its
> > > argument. I was concerned that it's going to be had to pick out whether
> > > !LOCKDEP builds should return true or false from LOCKDEP helpers, but
> > > perhaps relying on the linker errors even more is not such poor taste?
> > > 
> > > Does the patch below look acceptable to you?  
> > 
> > The thing to check would be whether all compilers do sufficient
> > dead-code elimination (it used to be that they did not).  One way to
> > get a quick sniff test of this would be to make sure that a dead-code
> > lockdep_is_held() is in common code, and then expose this patch to kbuild
> > test robot.
> 
> I'm pretty sure it's in common code because kbuild bot complaints were
> the reason I gave up the first time around ;) 
> 
> I'll expose this to kbuild bot via my kernel.org tree in case it
> doesn't consider scissored patches and report back!

Sounds good, thank you!

							Thanx, Paul