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 <[email protected]>
---
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
On 8 September 2020 20:36:24 EEST, Jakub Kicinski <[email protected]> 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 <[email protected]>
>---
> 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 <[email protected]>
On Tue, 08 Sep 2020 21:15:56 +0300 [email protected] wrote:
> Ah, you want to solve it for all. :)
> Looks and sounds good to me,
> Reviewed-by: Nikolay Aleksandrov <[email protected]>
Actually, I give up, lockdep_is_held() is not defined without
CONFIG_LOCKDEP, let's just go with your patch..
From: Jakub Kicinski <[email protected]>
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 <[email protected]>
I ended up applying Nikolay's fix, but this situation with the rcu macros
needs to be addressed.
On Tue, Sep 08, 2020 at 05:27:51PM -0700, Jakub Kicinski wrote:
> On Tue, 08 Sep 2020 21:15:56 +0300 [email protected] wrote:
> > Ah, you want to solve it for all. :)
> > Looks and sounds good to me,
> > Reviewed-by: Nikolay Aleksandrov <[email protected]>
>
> 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) <[email protected]>
thanks!
- Joel
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 [email protected] wrote:
> > > Ah, you want to solve it for all. :)
> > > Looks and sounds good to me,
> > > Reviewed-by: Nikolay Aleksandrov <[email protected]>
> >
> > 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 <[email protected]>
---
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
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 [email protected] wrote:
> > > > Ah, you want to solve it for all. :)
> > > > Looks and sounds good to me,
> > > > Reviewed-by: Nikolay Aleksandrov <[email protected]>
> > >
> > > 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 <[email protected]>
> ---
> 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
>
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!
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 [email protected] wrote:
> > > > Ah, you want to solve it for all. :)
> > > > Looks and sounds good to me,
> > > > Reviewed-by: Nikolay Aleksandrov <[email protected]>
> > >
> > > 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 <[email protected]>
> ---
> 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
>
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