From: Stephen Rothwell <hidden> Date: 2020-09-08 03:00:09
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
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-09-08 03:49:26
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!
From: Nikolay Aleksandrov <hidden> Date: 2020-09-08 07:17:33
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 <redacted>
Signed-off-by: Nikolay Aleksandrov <redacted>
---
net/bridge/br_multicast.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-09-08 16:20:07
On Tue, 8 Sep 2020 10:17:13 +0300 Nikolay Aleksandrov wrote:
quoted hunk
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 <redacted>
Signed-off-by: Nikolay Aleksandrov <redacted>
---
net/bridge/br_multicast.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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.
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-09-08 17:37:04
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(-)
On 8 September 2020 20:36:24 EEST, Jakub Kicinski [off-list ref] wrote:
quoted hunk
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(-)
On 8 September 2020 19:00:49 EEST, Jakub Kicinski [off-list ref] wrote:
On Tue, 8 Sep 2020 10:17:13 +0300 Nikolay Aleksandrov wrote:
quoted
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]
quoted
1818 | struct net_bridge *br = mp->br;
| ^~
It happens due to bridge's mlock_dereference() when lockdep isn't
defined.
quoted
Silence the warning by annotating the variable as __maybe_unused.
Fixes: 0436862e417e ("net: bridge: mcast: support for IGMPv3/MLDv2
ALLOW_NEW_SOURCES report")
quoted
Reported-by: Stephen Rothwell <redacted>
Signed-off-by: Nikolay Aleksandrov <redacted>
---
net/bridge/br_multicast.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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.
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.
From: Joel Fernandes <hidden> Date: 2020-09-14 20:22:48
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:
quoted
Ah, you want to solve it for all. :)
Looks and sounds good to me,
Reviewed-by: Nikolay Aleksandrov <redacted>
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) <redacted>
thanks!
- Joel
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-09-14 22:47:47
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:
quoted
On Tue, 08 Sep 2020 21:15:56 +0300 nikolay@cumulusnetworks.com wrote:
quoted
Ah, you want to solve it for all. :)
Looks and sounds good to me,
Reviewed-by: Nikolay Aleksandrov <redacted>
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(-)
From: "Paul E. McKenney" <paulmck@kernel.org> Date: 2020-09-15 00:20:17
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:
quoted
On Tue, Sep 08, 2020 at 05:27:51PM -0700, Jakub Kicinski wrote:
quoted
On Tue, 08 Sep 2020 21:15:56 +0300 nikolay@cumulusnetworks.com wrote:
quoted
Ah, you want to solve it for all. :)
Looks and sounds good to me,
Reviewed-by: Nikolay Aleksandrov <redacted>
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
quoted hunk
--->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(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-09-15 00:30:40
On Mon, 14 Sep 2020 17:20:11 -0700 Paul E. McKenney wrote:
quoted
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!
From: Joel Fernandes <hidden> Date: 2020-09-15 01:34:14
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:
quoted
On Tue, Sep 08, 2020 at 05:27:51PM -0700, Jakub Kicinski wrote:
quoted
On Tue, 08 Sep 2020 21:15:56 +0300 nikolay@cumulusnetworks.com wrote:
quoted
Ah, you want to solve it for all. :)
Looks and sounds good to me,
Reviewed-by: Nikolay Aleksandrov <redacted>
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
From: "Paul E. McKenney" <paulmck@kernel.org> Date: 2020-09-15 19:02:15
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:
quoted
quoted
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!