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@xxxxxxxxxxxxxxxxxxx wrote: > > > > Ah, you want to solve it for all. :) > > > > Looks and sounds good to me, > > > > Reviewed-by: Nikolay Aleksandrov <nikolay@xxxxxxxxxxxxxxxxxxx> > > > > > > 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@xxxxxxxxxx> > --- > 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 >