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

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

[Jakub Kicinski]

Hi!

So I unfolded the RFC patch into smaller chunks and fixed an issue
in SRCU pointed out by build bot. Build bot has been quiet for
a day but I'm not 100% sure it's scanning my tree, so let's
give these patches some ML exposure.

The motivation here is that we run into a unused variable
warning in networking code because RCU_LOCKDEP_WARN() makes
its argument disappear with !LOCKDEP / !PROVE_RCU. We marked
the variable as __maybe_unused, but that's ugly IMHO.

This set makes the relevant function declarations visible to
the compiler and uses (0 && (condition)) to make the compiler
remove those calls before linker realizes they are never defined.

I'm tentatively marking these for net-next, but if anyone (Paul?)
wants to take them into their tree - even better.

Jakub Kicinski (7):
  sched: un-hide lockdep_tasklist_lock_is_held() for !LOCKDEP
  rcu: un-hide lockdep maps for !LOCKDEP
  net: un-hide lockdep_sock_is_held() for !LOCKDEP
  net: sched: remove broken definitions and un-hide for !LOCKDEP
  srcu: use a more appropriate lockdep helper
  lockdep: provide dummy forward declaration of *_is_held() helpers
  rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition

 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 --
 kernel/rcu/srcutree.c          |  2 +-
 7 files changed, 15 insertions(+), 24 deletions(-)

-- 
2.26.2

[PATCH net-next 1/7] sched: un-hide lockdep_tasklist_lock_is_held() for !LOCKDEP

[Jakub Kicinski]

We're trying to make LOCKDEP-related function declarations
visible to the compiler and depend on dead code elimination
to remove them.

Make lockdep_tasklist_lock_is_held() visible.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: christian.brauner@ubuntu.com
CC: peterz@infradead.org
CC: ebiederm@xmission.com
CC: akpm@linux-foundation.org
CC: mingo@kernel.org
---
 include/linux/sched/task.h | 2 --
 1 file changed, 2 deletions(-)

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);
-- 
2.26.2

[PATCH net-next 2/7] rcu: un-hide lockdep maps for !LOCKDEP

[Jakub Kicinski]

We're trying to make LOCKDEP-related forward declarations
visible to the compiler and depend on dead code elimination
to remove them.

Expose RCU lock maps.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: paulmck@kernel.org
CC: josh@joshtriplett.org
CC: rostedt@goodmis.org
CC: mathieu.desnoyers@efficios.com
CC: joel@joelfernandes.org
CC: jiangshanlai@gmail.com
---
 include/linux/rcupdate.h       | 9 +++++----
 include/linux/rcupdate_trace.h | 4 ++--
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index d15d46db61f7..53f9648cb982 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);
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);
-- 
2.26.2

[PATCH net-next 3/7] net: un-hide lockdep_sock_is_held() for !LOCKDEP

[Jakub Kicinski]

We're trying to make LOCKDEP-related function declarations
visible to the compiler and depend on dead code elimination
to remove them.

Un-hide lockdep_sock_is_held().

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/sock.h | 2 --
 1 file changed, 2 deletions(-)

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.26.2

[PATCH net-next 4/7] net: sched: remove broken definitions and un-hide for !LOCKDEP

[Jakub Kicinski]

We're trying to make LOCKDEP-related function declarations
visible to the compiler and depend on dead code elimination
to remove them.

Fix up the situation with lockdep_tcf_chain_is_locked() and
lockdep_tcf_proto_is_locked().

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
CC: jhs@mojatatu.com
CC: xiyou.wangcong@gmail.com
CC: jiri@resnulli.us
---
 include/net/sch_generic.h | 12 ------------
 1 file changed, 12 deletions(-)

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

[PATCH net-next 5/7] srcu: use a more appropriate lockdep helper

[Jakub Kicinski]

lockdep_is_held() is defined as:

 #define lockdep_is_held(lock)		lock_is_held(&(lock)->dep_map)

it hides away the dereference, so that builds with !LOCKDEP
don't break. We should use it instead of using lock_is_held()
directly.

This didn't use to be a problem, because RCU_LOCKDEP_WARN()
cuts the condition out with the preprocessor if !LOCKDEP.
This will soon change.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
CC: jiangshanlai@gmail.com
CC: paulmck@kernel.org
CC: josh@joshtriplett.org
CC: rostedt@goodmis.org
CC: mathieu.desnoyers@efficios.com
---
 kernel/rcu/srcutree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index c100acf332ed..8123ef15a159 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -919,7 +919,7 @@ static void __synchronize_srcu(struct srcu_struct *ssp, bool do_norm)
 {
 	struct rcu_synchronize rcu;
 
-	RCU_LOCKDEP_WARN(lock_is_held(&ssp->dep_map) ||
+	RCU_LOCKDEP_WARN(lockdep_is_held(ssp) ||
 			 lock_is_held(&rcu_bh_lock_map) ||
 			 lock_is_held(&rcu_lock_map) ||
 			 lock_is_held(&rcu_sched_lock_map),
-- 
2.26.2

[PATCH net-next 6/7] lockdep: provide dummy forward declaration of *_is_held() helpers

[Jakub Kicinski]

When CONFIG_LOCKDEP is not set, lock_is_held() and lockdep_is_held()
are not declared or defined. This forces all callers to use ifdefs
around these checks.

Recent RCU changes added a lot of lockdep_is_held() calls inside
rcu_dereference_protected(). rcu_dereference_protected() hides
its argument on !LOCKDEP builds, but this may lead to unused variable
warnings.

Provide forward declarations of lock_is_held() and lockdep_is_held()
but never define them. This way callers can keep them visible to
the compiler on !LOCKDEP builds and instead depend on dead code
elimination to remove the references before the linker barfs.

We need lock_is_held() for RCU.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
CC: peterz@infradead.org
CC: mingo@redhat.com
CC: will@kernel.org
---
 include/linux/lockdep.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 6a584b3e5c74..6b5bbc536bf6 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)
-- 
2.26.2

[PATCH net-next 7/7] 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.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
CC: paulmck@kernel.org
CC: josh@joshtriplett.org
CC: rostedt@goodmis.org
CC: mathieu.desnoyers@efficios.com
CC: joel@joelfernandes.org
CC: jiangshanlai@gmail.com
---
 include/linux/rcupdate.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 53f9648cb982..50d45781fa99 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -321,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 */
-- 
2.26.2

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

[Paul E. McKenney]

On Wed, Sep 16, 2020 at 11:45:21AM -0700, Jakub Kicinski wrote:
> Hi!
> 
> So I unfolded the RFC patch into smaller chunks and fixed an issue
> in SRCU pointed out by build bot. Build bot has been quiet for
> a day but I'm not 100% sure it's scanning my tree, so let's
> give these patches some ML exposure.
> 
> The motivation here is that we run into a unused variable
> warning in networking code because RCU_LOCKDEP_WARN() makes
> its argument disappear with !LOCKDEP / !PROVE_RCU. We marked
> the variable as __maybe_unused, but that's ugly IMHO.
> 
> This set makes the relevant function declarations visible to
> the compiler and uses (0 && (condition)) to make the compiler
> remove those calls before linker realizes they are never defined.
> 
> I'm tentatively marking these for net-next, but if anyone (Paul?)
> wants to take them into their tree - even better.

I have pulled these into -rcu for review and further testing, thank you!
I of course could not resist editing the commit logs, so please check
to make sure that I did not mess anything up.  Just so you know, unless
this is urgent, it is in my v5.11 pile, that is, for the merge window
after next.

If someone else wants to take them, please feel free to add my
Acked-by to the RCU pieces.

							Thanx, Paul

> Jakub Kicinski (7):
>   sched: un-hide lockdep_tasklist_lock_is_held() for !LOCKDEP
>   rcu: un-hide lockdep maps for !LOCKDEP
>   net: un-hide lockdep_sock_is_held() for !LOCKDEP
>   net: sched: remove broken definitions and un-hide for !LOCKDEP
>   srcu: use a more appropriate lockdep helper
>   lockdep: provide dummy forward declaration of *_is_held() helpers
>   rcu: prevent RCU_LOCKDEP_WARN() from swallowing the condition
> 
>  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 --
>  kernel/rcu/srcutree.c          |  2 +-
>  7 files changed, 15 insertions(+), 24 deletions(-)
> 
> -- 
> 2.26.2
> 

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

[Jakub Kicinski]

On Wed, 16 Sep 2020 16:15:05 -0700 Paul E. McKenney wrote:
> On Wed, Sep 16, 2020 at 11:45:21AM -0700, Jakub Kicinski wrote:
> > Hi!
> > 
> > So I unfolded the RFC patch into smaller chunks and fixed an issue
> > in SRCU pointed out by build bot. Build bot has been quiet for
> > a day but I'm not 100% sure it's scanning my tree, so let's
> > give these patches some ML exposure.
> > 
> > The motivation here is that we run into a unused variable
> > warning in networking code because RCU_LOCKDEP_WARN() makes
> > its argument disappear with !LOCKDEP / !PROVE_RCU. We marked
> > the variable as __maybe_unused, but that's ugly IMHO.
> > 
> > This set makes the relevant function declarations visible to
> > the compiler and uses (0 && (condition)) to make the compiler
> > remove those calls before linker realizes they are never defined.
> > 
> > I'm tentatively marking these for net-next, but if anyone (Paul?)
> > wants to take them into their tree - even better.  
> 
> I have pulled these into -rcu for review and further testing, thank you!
> I of course could not resist editing the commit logs, so please check
> to make sure that I did not mess anything up.

Done & thank you!

> Just so you know, unless this is urgent, it is in my v5.11 pile, that
> is, for the merge window after next.
> 
> If someone else wants to take them, please feel free to add my
> Acked-by to the RCU pieces.

Sounds good to me, the RCU tree seems most appropriate and we added 
a workaround for the issue in net-next already, anyway.

Thanks!

Re: [PATCH net-next 6/7] lockdep: provide dummy forward declaration of *_is_held() helpers

[peterz]

On Wed, Sep 16, 2020 at 11:45:27AM -0700, Jakub Kicinski wrote:
> When CONFIG_LOCKDEP is not set, lock_is_held() and lockdep_is_held()
> are not declared or defined. This forces all callers to use ifdefs
> around these checks.
> 
> Recent RCU changes added a lot of lockdep_is_held() calls inside
> rcu_dereference_protected(). rcu_dereference_protected() hides
> its argument on !LOCKDEP builds, but this may lead to unused variable
> warnings.
> 
> Provide forward declarations of lock_is_held() and lockdep_is_held()
> but never define them. This way callers can keep them visible to
> the compiler on !LOCKDEP builds and instead depend on dead code
> elimination to remove the references before the linker barfs.
> 
> We need lock_is_held() for RCU.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> --
> CC: peterz@infradead.org
> CC: mingo@redhat.com
> CC: will@kernel.org
> ---
>  include/linux/lockdep.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 6a584b3e5c74..6b5bbc536bf6 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 *);

extern int lock_is_held(const struct lockdep_map *);

> +int lockdep_is_held(const void *);

extern

I suppose we can't pull the lockdep_is_held() definition out from under
CONFIG_LOCKDEP because it does the ->dep_map dereference and many types
will simply not have that member.

>  #define lockdep_is_held_type(l, r)		(1)
>  
>  #define lockdep_assert_held(l)			do { (void)(l); } while (0)