[PATCH] lockdep: Fix wait context check on softirq for PREEMPT_RT

[PATCH] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Ryo Takakura]

Commit 0c1d7a2c2d32 ("lockdep: Remove softirq accounting on
PREEMPT_RT.") stopped updating @softirq_context on PREEMPT_RT
to ignore "inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage"
as the report accounts softirq context which PREEMPT_RT doesn't
have to.

However, wait context check still needs to report mutex usage
within softirq, even when its threaded on PREEMPT_RT. The check
is failing to report the usage as task_wait_context() checks if
its in softirq by referencing @softirq_context, ending up not 
assigning the correct wait type of LD_WAIT_CONFIG for PREEMPT_RT's
softirq.

[    0.184549]   | wait context tests |
[    0.184549]   --------------------------------------------------------------------------
[    0.184549]                                  | rcu  | raw  | spin |mutex |
[    0.184549]   --------------------------------------------------------------------------
[    0.184550]                in hardirq context:  ok  |  ok  |  ok  |  ok  |
[    0.185083] in hardirq context (not threaded):  ok  |  ok  |  ok  |  ok  |
[    0.185606]                in softirq context:  ok  |  ok  |  ok  |FAILED|

Account softirq context but only when !PREEMPT_RT so that
task_wait_context() returns LD_WAIT_CONFIG as intended.

Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com>


---

Hi! 

I wasn't able come up with a way to fix the wait context test while 
keeping the commit 0c1d7a2c2d32 ("lockdep: Remove softirq accounting 
on PREEMPT_RT.") without referencing @softirq_context...
Hoping to get a feedback on it!

Also I wonder if the test can be skipped as I believe its taken care 
by spinlock wait context test since the PREEMPT_RT's softirq context is 
protected by local_lock which is mapped to rt_spinlock.

Thanks!
Ryo Takakura

---
 include/linux/irqflags.h |  2 +-
 kernel/locking/lockdep.c | 11 +++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 3f003d5fd..c33c3bbd8 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -121,7 +121,7 @@ do {						\
 # define lockdep_irq_work_exit(__work)		do { } while (0)
 #endif
 
-#if defined(CONFIG_TRACE_IRQFLAGS) && !defined(CONFIG_PREEMPT_RT)
+#if defined(CONFIG_TRACE_IRQFLAGS)
 # define lockdep_softirq_enter()		\
 do {						\
 	current->softirq_context++;		\
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 536bd4715..2a508d6a6 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4602,7 +4602,7 @@ mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
 				if (!mark_lock(curr, hlock,
 						LOCK_USED_IN_HARDIRQ_READ))
 					return 0;
-			if (curr->softirq_context)
+			if (!IS_ENABLED(CONFIG_PREEMPT_RT) && curr->softirq_context)
 				if (!mark_lock(curr, hlock,
 						LOCK_USED_IN_SOFTIRQ_READ))
 					return 0;
@@ -4610,7 +4610,7 @@ mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
 			if (lockdep_hardirq_context())
 				if (!mark_lock(curr, hlock, LOCK_USED_IN_HARDIRQ))
 					return 0;
-			if (curr->softirq_context)
+			if (!IS_ENABLED(CONFIG_PREEMPT_RT) && curr->softirq_context)
 				if (!mark_lock(curr, hlock, LOCK_USED_IN_SOFTIRQ))
 					return 0;
 		}
@@ -4651,8 +4651,11 @@ mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
 
 static inline unsigned int task_irq_context(struct task_struct *task)
 {
-	return LOCK_CHAIN_HARDIRQ_CONTEXT * !!lockdep_hardirq_context() +
-	       LOCK_CHAIN_SOFTIRQ_CONTEXT * !!task->softirq_context;
+	if (IS_ENABLED(CONFIG_PREEMPT_RT))
+		return LOCK_CHAIN_HARDIRQ_CONTEXT * !!lockdep_hardirq_context();
+	else
+		return LOCK_CHAIN_HARDIRQ_CONTEXT * !!lockdep_hardirq_context() +
+		       LOCK_CHAIN_SOFTIRQ_CONTEXT * !!task->softirq_context;
 }
 
 static int separate_irq_context(struct task_struct *curr,
-- 
2.34.1

Re: [PATCH] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Peter Zijlstra]

On Mon, Dec 02, 2024 at 10:20:17AM +0900, Ryo Takakura wrote:
> Commit 0c1d7a2c2d32 ("lockdep: Remove softirq accounting on
> PREEMPT_RT.") stopped updating @softirq_context on PREEMPT_RT
> to ignore "inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage"
> as the report accounts softirq context which PREEMPT_RT doesn't
> have to.
> 
> However, wait context check still needs to report mutex usage
> within softirq, even when its threaded on PREEMPT_RT. The check
> is failing to report the usage as task_wait_context() checks if
> its in softirq by referencing @softirq_context, ending up not 
> assigning the correct wait type of LD_WAIT_CONFIG for PREEMPT_RT's
> softirq.
> 
> [    0.184549]   | wait context tests |
> [    0.184549]   --------------------------------------------------------------------------
> [    0.184549]                                  | rcu  | raw  | spin |mutex |
> [    0.184549]   --------------------------------------------------------------------------
> [    0.184550]                in hardirq context:  ok  |  ok  |  ok  |  ok  |
> [    0.185083] in hardirq context (not threaded):  ok  |  ok  |  ok  |  ok  |
> [    0.185606]                in softirq context:  ok  |  ok  |  ok  |FAILED|
> 
> Account softirq context but only when !PREEMPT_RT so that
> task_wait_context() returns LD_WAIT_CONFIG as intended.
> 
> Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com>
> 
> 
> ---
> 
> Hi! 
> 
> I wasn't able come up with a way to fix the wait context test while 
> keeping the commit 0c1d7a2c2d32 ("lockdep: Remove softirq accounting 
> on PREEMPT_RT.") without referencing @softirq_context...
> Hoping to get a feedback on it!
> 
> Also I wonder if the test can be skipped as I believe its taken care 
> by spinlock wait context test since the PREEMPT_RT's softirq context is 
> protected by local_lock which is mapped to rt_spinlock.

Right,.. so I remember talking about this with Boqun, and I think we
were going to 'fix' the test, but I can't quite remember.

Perhaps adding the local_lock to SOFTIRQ_ENTER?

Re: [PATCH] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Boqun Feng]

On Mon, Dec 02, 2024 at 11:32:28AM +0100, Peter Zijlstra wrote:
> On Mon, Dec 02, 2024 at 10:20:17AM +0900, Ryo Takakura wrote:
> > Commit 0c1d7a2c2d32 ("lockdep: Remove softirq accounting on
> > PREEMPT_RT.") stopped updating @softirq_context on PREEMPT_RT
> > to ignore "inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage"
> > as the report accounts softirq context which PREEMPT_RT doesn't
> > have to.
> > 
> > However, wait context check still needs to report mutex usage
> > within softirq, even when its threaded on PREEMPT_RT. The check
> > is failing to report the usage as task_wait_context() checks if
> > its in softirq by referencing @softirq_context, ending up not 
> > assigning the correct wait type of LD_WAIT_CONFIG for PREEMPT_RT's
> > softirq.
> > 
> > [    0.184549]   | wait context tests |
> > [    0.184549]   --------------------------------------------------------------------------
> > [    0.184549]                                  | rcu  | raw  | spin |mutex |
> > [    0.184549]   --------------------------------------------------------------------------
> > [    0.184550]                in hardirq context:  ok  |  ok  |  ok  |  ok  |
> > [    0.185083] in hardirq context (not threaded):  ok  |  ok  |  ok  |  ok  |
> > [    0.185606]                in softirq context:  ok  |  ok  |  ok  |FAILED|
> > 
> > Account softirq context but only when !PREEMPT_RT so that
> > task_wait_context() returns LD_WAIT_CONFIG as intended.
> > 
> > Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com>
> > 
> > 
> > ---
> > 
> > Hi! 
> > 
> > I wasn't able come up with a way to fix the wait context test while 
> > keeping the commit 0c1d7a2c2d32 ("lockdep: Remove softirq accounting 
> > on PREEMPT_RT.") without referencing @softirq_context...
> > Hoping to get a feedback on it!
> > 
> > Also I wonder if the test can be skipped as I believe its taken care 

Skipping the test would be awful because tests are supposed to catch
unexpected bugs :/

> > by spinlock wait context test since the PREEMPT_RT's softirq context is 
> > protected by local_lock which is mapped to rt_spinlock.
> 
> Right,.. so I remember talking about this with Boqun, and I think we
> were going to 'fix' the test, but I can't quite remember.
> 
> Perhaps adding the local_lock to SOFTIRQ_ENTER?

So I took a look, SOFTIRQ_ENTER() already calls local_bh_disable(),
which is supposed to acquire a local_lock "softirq_ctrl.lock" (Ryo, I
believe this is the local_lock you mentioned above?) in normal cases.
However, if local_bh_disable() is called with preempt disabled, then no
local_lock will be acquired. For example, if you do:

	preempt_disable();
	local_bh_disable();
	preempt_enable();
	mutex_lock();

no local_lock will be acquired, therefore check_wait_context() will
report nothing. The fun part of "why this caused an issue in the lockdep
selftests?" is these tests are run with preempt_count() == 1 ;-) I guess
this is because we run these in early stage of kernel booting? Will take
a look tomorrow.

Maybe the right way to fix this is adding a conceptual local_lock for
BH disable like below.

Regards,
Boqun

------------------------->8
diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h
index fc53e0ad56d9..d5b898588277 100644
--- a/include/linux/bottom_half.h
+++ b/include/linux/bottom_half.h
@@ -4,6 +4,7 @@
 
 #include <linux/instruction_pointer.h>
 #include <linux/preempt.h>
+#include <linux/lockdep.h>
 
 #if defined(CONFIG_PREEMPT_RT) || defined(CONFIG_TRACE_IRQFLAGS)
 extern void __local_bh_disable_ip(unsigned long ip, unsigned int cnt);
@@ -15,9 +16,12 @@ static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned int
 }
 #endif
 
+extern struct lockdep_map bh_lock_map;
+
 static inline void local_bh_disable(void)
 {
 	__local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
+	lock_map_acquire(&bh_lock_map);
 }
 
 extern void _local_bh_enable(void);
@@ -25,6 +29,7 @@ extern void __local_bh_enable_ip(unsigned long ip, unsigned int cnt);
 
 static inline void local_bh_enable_ip(unsigned long ip)
 {
+	lock_map_release(&bh_lock_map);
 	__local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET);
 }
 
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 8b41bd13cc3d..17d9bf6e0caf 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -1066,3 +1066,13 @@ unsigned int __weak arch_dynirq_lower_bound(unsigned int from)
 {
 	return from;
 }
+
+static struct lock_class_key bh_lock_key;
+struct lockdep_map bh_lock_map = {
+	.name = "local_bh",
+	.key = &bh_lock_key,
+	.wait_type_outer = LD_WAIT_FREE,
+	.wait_type_inner = LD_WAIT_CONFIG, /* PREEMPT_RT makes BH preemptible. */
+	.lock_type = LD_LOCK_PERCPU,
+};
+EXPORT_SYMBOL_GPL(bh_lock_map);

Re: [PATCH] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Ryo Takakura]

Hi Peter and Boqun,
Thanks for getting back!

On Mon, 2 Dec 2024 23:49:24 -0800, Boqun Feng wrote:
>On Mon, Dec 02, 2024 at 11:32:28AM +0100, Peter Zijlstra wrote:
>> On Mon, Dec 02, 2024 at 10:20:17AM +0900, Ryo Takakura wrote:
>> > Commit 0c1d7a2c2d32 ("lockdep: Remove softirq accounting on
>> > PREEMPT_RT.") stopped updating @softirq_context on PREEMPT_RT
>> > to ignore "inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage"
>> > as the report accounts softirq context which PREEMPT_RT doesn't
>> > have to.
>> > 
>> > However, wait context check still needs to report mutex usage
>> > within softirq, even when its threaded on PREEMPT_RT. The check
>> > is failing to report the usage as task_wait_context() checks if
>> > its in softirq by referencing @softirq_context, ending up not 
>> > assigning the correct wait type of LD_WAIT_CONFIG for PREEMPT_RT's
>> > softirq.
>> > 
>> > [    0.184549]   | wait context tests |
>> > [    0.184549]   --------------------------------------------------------------------------
>> > [    0.184549]                                  | rcu  | raw  | spin |mutex |
>> > [    0.184549]   --------------------------------------------------------------------------
>> > [    0.184550]                in hardirq context:  ok  |  ok  |  ok  |  ok  |
>> > [    0.185083] in hardirq context (not threaded):  ok  |  ok  |  ok  |  ok  |
>> > [    0.185606]                in softirq context:  ok  |  ok  |  ok  |FAILED|
>> > 
>> > Account softirq context but only when !PREEMPT_RT so that
>> > task_wait_context() returns LD_WAIT_CONFIG as intended.
>> > 
>> > Signed-off-by: Ryo Takakura <ryotkkr98@gmail.com>
>> > 
>> > 
>> > ---
>> > 
>> > Hi! 
>> > 
>> > I wasn't able come up with a way to fix the wait context test while 
>> > keeping the commit 0c1d7a2c2d32 ("lockdep: Remove softirq accounting 
>> > on PREEMPT_RT.") without referencing @softirq_context...
>> > Hoping to get a feedback on it!
>> > 
>> > Also I wonder if the test can be skipped as I believe its taken care 
>
>Skipping the test would be awful because tests are supposed to catch
>unexpected bugs :/
>
>> > by spinlock wait context test since the PREEMPT_RT's softirq context is 
>> > protected by local_lock which is mapped to rt_spinlock.
>> 
>> Right,.. so I remember talking about this with Boqun, and I think we
>> were going to 'fix' the test, but I can't quite remember.
>> 
>> Perhaps adding the local_lock to SOFTIRQ_ENTER?
>
>So I took a look, SOFTIRQ_ENTER() already calls local_bh_disable(),
>which is supposed to acquire a local_lock "softirq_ctrl.lock" (Ryo, I
>believe this is the local_lock you mentioned above?) in normal cases.

Yes, and I was assuming the normal case...

Since Peter's feedback, I was just wondering why the wait context 
selftest was not reporting anything if the local_lock were already 
acquired (answered below!).

>However, if local_bh_disable() is called with preempt disabled, then no
>local_lock will be acquired. For example, if you do:
>
>	preempt_disable();
>	local_bh_disable();
>	preempt_enable();
>	mutex_lock();
>
>no local_lock will be acquired, therefore check_wait_context() will
>report nothing. The fun part of "why this caused an issue in the lockdep
>selftests?" is these tests are run with preempt_count() == 1 ;-) I guess
>this is because we run these in early stage of kernel booting? Will take
>a look tomorrow.

I see! That is indeed quite fun!

>Maybe the right way to fix this is adding a conceptual local_lock for
>BH disable like below.
>
>Regards,
>Boqun
>
>------------------------->8
>diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h
>index fc53e0ad56d9..d5b898588277 100644
>--- a/include/linux/bottom_half.h
>+++ b/include/linux/bottom_half.h
>@@ -4,6 +4,7 @@
> 
> #include <linux/instruction_pointer.h>
> #include <linux/preempt.h>
>+#include <linux/lockdep.h>
> 
> #if defined(CONFIG_PREEMPT_RT) || defined(CONFIG_TRACE_IRQFLAGS)
> extern void __local_bh_disable_ip(unsigned long ip, unsigned int cnt);
>@@ -15,9 +16,12 @@ static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned int
> }
> #endif
> 
>+extern struct lockdep_map bh_lock_map;
>+
> static inline void local_bh_disable(void)
> {
> 	__local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
>+	lock_map_acquire(&bh_lock_map);
> }
> 
> extern void _local_bh_enable(void);
>@@ -25,6 +29,7 @@ extern void __local_bh_enable_ip(unsigned long ip, unsigned int cnt);
> 
> static inline void local_bh_enable_ip(unsigned long ip)
> {
>+	lock_map_release(&bh_lock_map);
> 	__local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET);
> }
> 
>diff --git a/kernel/softirq.c b/kernel/softirq.c
>index 8b41bd13cc3d..17d9bf6e0caf 100644
>--- a/kernel/softirq.c
>+++ b/kernel/softirq.c
>@@ -1066,3 +1066,13 @@ unsigned int __weak arch_dynirq_lower_bound(unsigned int from)
> {
> 	return from;
> }
>+
>+static struct lock_class_key bh_lock_key;
>+struct lockdep_map bh_lock_map = {
>+	.name = "local_bh",
>+	.key = &bh_lock_key,
>+	.wait_type_outer = LD_WAIT_FREE,
>+	.wait_type_inner = LD_WAIT_CONFIG, /* PREEMPT_RT makes BH preemptible. */
>+	.lock_type = LD_LOCK_PERCPU,
>+};
>+EXPORT_SYMBOL_GPL(bh_lock_map);

Let me take a look at it!

Sincerely,
Ryo Takakura

Re: [PATCH] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Ryo Takakura]

Hi!

Sorry for being late on reply. I was trying to understand some 
of the selftest reports that came across...

On Mon, 2 Dec 2024 23:49:24 -0800, Boqun Feng wrote:
>Maybe the right way to fix this is adding a conceptual local_lock for
>BH disable like below.
>
>Regards,
>Boqun
>
>------------------------->8
>diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h
>index fc53e0ad56d9..d5b898588277 100644
>--- a/include/linux/bottom_half.h
>+++ b/include/linux/bottom_half.h
>@@ -4,6 +4,7 @@
> 
> #include <linux/instruction_pointer.h>
> #include <linux/preempt.h>
>+#include <linux/lockdep.h>
> 
> #if defined(CONFIG_PREEMPT_RT) || defined(CONFIG_TRACE_IRQFLAGS)
> extern void __local_bh_disable_ip(unsigned long ip, unsigned int cnt);
>@@ -15,9 +16,12 @@ static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned int
> }
> #endif
> 
>+extern struct lockdep_map bh_lock_map;
>+
> static inline void local_bh_disable(void)
> {
> 	__local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
>+	lock_map_acquire(&bh_lock_map);
> }
> 
> extern void _local_bh_enable(void);
>@@ -25,6 +29,7 @@ extern void __local_bh_enable_ip(unsigned long ip, unsigned int cnt);
> 
> static inline void local_bh_enable_ip(unsigned long ip)
> {
>+	lock_map_release(&bh_lock_map);
> 	__local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET);
> }

Maybe &bh_lock_map should be acquired at local_bh_enable()?

 static inline void local_bh_enable(void)
 {
+       lock_map_release(&bh_lock_map);
        __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
 }

On !CONFIG_PREEMPT_RT, I noticed that softirq related selftests
(e.g. lock-inversion) fails with recursive locking error

[    0.741422] ============================================
[    0.741447] WARNING: possible recursive locking detected
[    0.741471] 6.12.0-rc1-v8+ #25 Not tainted
[    0.741495] --------------------------------------------
[    0.741519] swapper/0/0 is trying to acquire lock:
[    0.741544] ffffffecd02e0160 (local_bh){+.+.}-{1:3}, at: irq_inversion_soft_spin_123+0x0/0x178
[    0.741621]
               but task is already holding lock:
[    0.741648] ffffffecd02e0160 (local_bh){+.+.}-{1:3}, at: irq_inversion_soft_spin_123+0x0/0x178
[    0.741721]
               other info that might help us debug this:
[    0.741750]  Possible unsafe locking scenario:

[    0.741776]        CPU0
[    0.741793]        ----
[    0.741810]   lock(local_bh);
[    0.741840]   lock(local_bh);
[    0.741868]
                *** DEADLOCK ***

where it does SOFTIRQ_ENTER()/EXIT() and SOFTIRQ_DISABLE()/ENABLE() 
as each enables BH with local_bh_enable().

But I was little confused that isn't recursively disabling BH allowed, 
especially if PREEMPT_RT doesn't disable preemption? (I was also 
wondering if disabling BH recursively is something that can happen 
on !PREEMPT_RT if it disables preemption...)
If so, wouldn't report for such case be false?

>diff --git a/kernel/softirq.c b/kernel/softirq.c
>index 8b41bd13cc3d..17d9bf6e0caf 100644
>--- a/kernel/softirq.c
>+++ b/kernel/softirq.c
>@@ -1066,3 +1066,13 @@ unsigned int __weak arch_dynirq_lower_bound(unsigned int from)
> {
> 	return from;
> }
>+
>+static struct lock_class_key bh_lock_key;
>+struct lockdep_map bh_lock_map = {
>+	.name = "local_bh",
>+	.key = &bh_lock_key,
>+	.wait_type_outer = LD_WAIT_FREE,
>+	.wait_type_inner = LD_WAIT_CONFIG, /* PREEMPT_RT makes BH preemptible. */
>+	.lock_type = LD_LOCK_PERCPU,
>+};
>+EXPORT_SYMBOL_GPL(bh_lock_map);

Sincerely,
Ryo Takakura

Re: [PATCH] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Boqun Feng]

On Tue, Dec 10, 2024 at 01:09:43AM +0900, Ryo Takakura wrote:
> Hi!
> 
> Sorry for being late on reply. I was trying to understand some 
> of the selftest reports that came across...
> 
> On Mon, 2 Dec 2024 23:49:24 -0800, Boqun Feng wrote:
> >Maybe the right way to fix this is adding a conceptual local_lock for
> >BH disable like below.
> >
> >Regards,
> >Boqun
> >
> >------------------------->8
> >diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h
> >index fc53e0ad56d9..d5b898588277 100644
> >--- a/include/linux/bottom_half.h
> >+++ b/include/linux/bottom_half.h
> >@@ -4,6 +4,7 @@
> > 
> > #include <linux/instruction_pointer.h>
> > #include <linux/preempt.h>
> >+#include <linux/lockdep.h>
> > 
> > #if defined(CONFIG_PREEMPT_RT) || defined(CONFIG_TRACE_IRQFLAGS)
> > extern void __local_bh_disable_ip(unsigned long ip, unsigned int cnt);
> >@@ -15,9 +16,12 @@ static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned int
> > }
> > #endif
> > 
> >+extern struct lockdep_map bh_lock_map;
> >+
> > static inline void local_bh_disable(void)
> > {
> > 	__local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
> >+	lock_map_acquire(&bh_lock_map);
> > }
> > 
> > extern void _local_bh_enable(void);
> >@@ -25,6 +29,7 @@ extern void __local_bh_enable_ip(unsigned long ip, unsigned int cnt);
> > 
> > static inline void local_bh_enable_ip(unsigned long ip)
> > {
> >+	lock_map_release(&bh_lock_map);
> > 	__local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET);
> > }
> 
> Maybe &bh_lock_map should be acquired at local_bh_enable()?
> 

Right, local_bh_enable_ip() seems to be an unused function...

>  static inline void local_bh_enable(void)
>  {
> +       lock_map_release(&bh_lock_map);
>         __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
>  }
> 
> On !CONFIG_PREEMPT_RT, I noticed that softirq related selftests
> (e.g. lock-inversion) fails with recursive locking error
> 
> [    0.741422] ============================================
> [    0.741447] WARNING: possible recursive locking detected
> [    0.741471] 6.12.0-rc1-v8+ #25 Not tainted
> [    0.741495] --------------------------------------------
> [    0.741519] swapper/0/0 is trying to acquire lock:
> [    0.741544] ffffffecd02e0160 (local_bh){+.+.}-{1:3}, at: irq_inversion_soft_spin_123+0x0/0x178
> [    0.741621]
>                but task is already holding lock:
> [    0.741648] ffffffecd02e0160 (local_bh){+.+.}-{1:3}, at: irq_inversion_soft_spin_123+0x0/0x178
> [    0.741721]
>                other info that might help us debug this:
> [    0.741750]  Possible unsafe locking scenario:
> 
> [    0.741776]        CPU0
> [    0.741793]        ----
> [    0.741810]   lock(local_bh);
> [    0.741840]   lock(local_bh);
> [    0.741868]
>                 *** DEADLOCK ***
> 
> where it does SOFTIRQ_ENTER()/EXIT() and SOFTIRQ_DISABLE()/ENABLE() 
> as each enables BH with local_bh_enable().
> 
> But I was little confused that isn't recursively disabling BH allowed, 
> especially if PREEMPT_RT doesn't disable preemption? (I was also 
> wondering if disabling BH recursively is something that can happen 
> on !PREEMPT_RT if it disables preemption...)
> If so, wouldn't report for such case be false?
> 

I think you're right. Recursively BH disabling should be allowed, what
I missed was that we should use lock_map_acquire_read() instead of
lock_map_acquire() in local_bh_disable(). Because as you said,
recursively BH disabling should be allowed therefore the virtual "lock"
of BH disabling should be a read lock.

The following is the rough idea:

Regards,
Boqun

----->8
diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h
index fc53e0ad56d9..7191a753e983 100644
--- a/include/linux/bottom_half.h
+++ b/include/linux/bottom_half.h
@@ -4,6 +4,7 @@
 
 #include <linux/instruction_pointer.h>
 #include <linux/preempt.h>
+#include <linux/lockdep.h>
 
 #if defined(CONFIG_PREEMPT_RT) || defined(CONFIG_TRACE_IRQFLAGS)
 extern void __local_bh_disable_ip(unsigned long ip, unsigned int cnt);
@@ -15,9 +16,12 @@ static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned int
 }
 #endif
 
+extern struct lockdep_map bh_lock_map;
+
 static inline void local_bh_disable(void)
 {
 	__local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
+	lock_map_acquire_read(&bh_lock_map);
 }
 
 extern void _local_bh_enable(void);
@@ -25,11 +29,13 @@ extern void __local_bh_enable_ip(unsigned long ip, unsigned int cnt);
 
 static inline void local_bh_enable_ip(unsigned long ip)
 {
+	lock_map_release(&bh_lock_map);
 	__local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET);
 }
 
 static inline void local_bh_enable(void)
 {
+	lock_map_release(&bh_lock_map);
 	__local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
 }
 
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 8b41bd13cc3d..17d9bf6e0caf 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -1066,3 +1066,13 @@ unsigned int __weak arch_dynirq_lower_bound(unsigned int from)
 {
 	return from;
 }
+
+static struct lock_class_key bh_lock_key;
+struct lockdep_map bh_lock_map = {
+	.name = "local_bh",
+	.key = &bh_lock_key,
+	.wait_type_outer = LD_WAIT_FREE,
+	.wait_type_inner = LD_WAIT_CONFIG, /* PREEMPT_RT makes BH preemptible. */
+	.lock_type = LD_LOCK_PERCPU,
+};
+EXPORT_SYMBOL_GPL(bh_lock_map);

Re: [PATCH] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Sebastian Andrzej Siewior]

On 2024-12-19 14:27:11 [-0800], Boqun Feng wrote:
> diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h
> index fc53e0ad56d9..7191a753e983 100644
> --- a/include/linux/bottom_half.h
> +++ b/include/linux/bottom_half.h
> @@ -4,6 +4,7 @@
>  
>  #include <linux/instruction_pointer.h>
>  #include <linux/preempt.h>
> +#include <linux/lockdep.h>
>  
>  #if defined(CONFIG_PREEMPT_RT) || defined(CONFIG_TRACE_IRQFLAGS)
>  extern void __local_bh_disable_ip(unsigned long ip, unsigned int cnt);
> @@ -15,9 +16,12 @@ static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned int
>  }
>  #endif
>  
> +extern struct lockdep_map bh_lock_map;
> +
>  static inline void local_bh_disable(void)
>  {
>  	__local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
> +	lock_map_acquire_read(&bh_lock_map);
>  }

Could you put this before __local_bh_disable_ip(), please?

Sebastian

Re: [PATCH] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Ryo Takakura]

On Thu, 19 Dec 2024 14:27:11 -0800, Boqun Feng wrote:
>The following is the rough idea:
>
>Regards,
>Boqun
>
>----->8
>diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h
>index fc53e0ad56d9..7191a753e983 100644
>--- a/include/linux/bottom_half.h
>+++ b/include/linux/bottom_half.h
>@@ -4,6 +4,7 @@
> 
> #include <linux/instruction_pointer.h>
> #include <linux/preempt.h>
>+#include <linux/lockdep.h>
> 
> #if defined(CONFIG_PREEMPT_RT) || defined(CONFIG_TRACE_IRQFLAGS)
> extern void __local_bh_disable_ip(unsigned long ip, unsigned int cnt);
>@@ -15,9 +16,12 @@ static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned int
> }
> #endif
> 
>+extern struct lockdep_map bh_lock_map;
>+
> static inline void local_bh_disable(void)
> {
> 	__local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
>+	lock_map_acquire_read(&bh_lock_map);
> }
> 
> extern void _local_bh_enable(void);
>@@ -25,11 +29,13 @@ extern void __local_bh_enable_ip(unsigned long ip, unsigned int cnt);
> 
> static inline void local_bh_enable_ip(unsigned long ip)
> {
>+	lock_map_release(&bh_lock_map);
> 	__local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET);
> }
> 
> static inline void local_bh_enable(void)
> {
>+	lock_map_release(&bh_lock_map);
> 	__local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
> }
> 
>diff --git a/kernel/softirq.c b/kernel/softirq.c
>index 8b41bd13cc3d..17d9bf6e0caf 100644
>--- a/kernel/softirq.c
>+++ b/kernel/softirq.c
>@@ -1066,3 +1066,13 @@ unsigned int __weak arch_dynirq_lower_bound(unsigned int from)
> {
> 	return from;
> }
>+
>+static struct lock_class_key bh_lock_key;
>+struct lockdep_map bh_lock_map = {
>+	.name = "local_bh",
>+	.key = &bh_lock_key,
>+	.wait_type_outer = LD_WAIT_FREE,
>+	.wait_type_inner = LD_WAIT_CONFIG, /* PREEMPT_RT makes BH preemptible. */
>+	.lock_type = LD_LOCK_PERCPU,
>+};
>+EXPORT_SYMBOL_GPL(bh_lock_map);

Self-tests are now all passing. Thanks!
It looks good to me.

Sincerely,
Ryo Takakura

Re: [PATCH] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Boqun Feng]

On Sat, Dec 21, 2024 at 01:10:16AM +0900, Ryo Takakura wrote:
> On Thu, 19 Dec 2024 14:27:11 -0800, Boqun Feng wrote:
> >The following is the rough idea:
> >
> >Regards,
> >Boqun
> >
> >----->8
> >diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h
> >index fc53e0ad56d9..7191a753e983 100644
> >--- a/include/linux/bottom_half.h
> >+++ b/include/linux/bottom_half.h
> >@@ -4,6 +4,7 @@
> > 
> > #include <linux/instruction_pointer.h>
> > #include <linux/preempt.h>
> >+#include <linux/lockdep.h>
> > 
> > #if defined(CONFIG_PREEMPT_RT) || defined(CONFIG_TRACE_IRQFLAGS)
> > extern void __local_bh_disable_ip(unsigned long ip, unsigned int cnt);
> >@@ -15,9 +16,12 @@ static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned int
> > }
> > #endif
> > 
> >+extern struct lockdep_map bh_lock_map;
> >+
> > static inline void local_bh_disable(void)
> > {
> > 	__local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
> >+	lock_map_acquire_read(&bh_lock_map);
> > }
> > 
> > extern void _local_bh_enable(void);
> >@@ -25,11 +29,13 @@ extern void __local_bh_enable_ip(unsigned long ip, unsigned int cnt);
> > 
> > static inline void local_bh_enable_ip(unsigned long ip)
> > {
> >+	lock_map_release(&bh_lock_map);
> > 	__local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET);
> > }
> > 
> > static inline void local_bh_enable(void)
> > {
> >+	lock_map_release(&bh_lock_map);
> > 	__local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
> > }
> > 
> >diff --git a/kernel/softirq.c b/kernel/softirq.c
> >index 8b41bd13cc3d..17d9bf6e0caf 100644
> >--- a/kernel/softirq.c
> >+++ b/kernel/softirq.c
> >@@ -1066,3 +1066,13 @@ unsigned int __weak arch_dynirq_lower_bound(unsigned int from)
> > {
> > 	return from;
> > }
> >+
> >+static struct lock_class_key bh_lock_key;
> >+struct lockdep_map bh_lock_map = {
> >+	.name = "local_bh",
> >+	.key = &bh_lock_key,
> >+	.wait_type_outer = LD_WAIT_FREE,
> >+	.wait_type_inner = LD_WAIT_CONFIG, /* PREEMPT_RT makes BH preemptible. */
> >+	.lock_type = LD_LOCK_PERCPU,
> >+};
> >+EXPORT_SYMBOL_GPL(bh_lock_map);
> 
> Self-tests are now all passing. Thanks!
> It looks good to me.

Good! Do you want to continue working on this based on the above
changes? If so, feel free.

Regards,
Boqun

> 
> Sincerely,
> Ryo Takakura

Re: [PATCH] lockdep: Fix wait context check on softirq for PREEMPT_RT

[Ryo Takakura]

On Fri, 20 Dec 2024 11:00:13 -0800, Boqun Feng wrote:
>On Sat, Dec 21, 2024 at 01:10:16AM +0900, Ryo Takakura wrote:
>> On Thu, 19 Dec 2024 14:27:11 -0800, Boqun Feng wrote:
>> >The following is the rough idea:
>> >
>> >Regards,
>> >Boqun
>> >
>> >----->8
>> >diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h
>> >index fc53e0ad56d9..7191a753e983 100644
>> >--- a/include/linux/bottom_half.h
>> >+++ b/include/linux/bottom_half.h
>> >@@ -4,6 +4,7 @@
>> > 
>> > #include <linux/instruction_pointer.h>
>> > #include <linux/preempt.h>
>> >+#include <linux/lockdep.h>
>> > 
>> > #if defined(CONFIG_PREEMPT_RT) || defined(CONFIG_TRACE_IRQFLAGS)
>> > extern void __local_bh_disable_ip(unsigned long ip, unsigned int cnt);
>> >@@ -15,9 +16,12 @@ static __always_inline void __local_bh_disable_ip(unsigned long ip, unsigned int
>> > }
>> > #endif
>> > 
>> >+extern struct lockdep_map bh_lock_map;
>> >+
>> > static inline void local_bh_disable(void)
>> > {
>> > 	__local_bh_disable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
>> >+	lock_map_acquire_read(&bh_lock_map);
>> > }
>> > 
>> > extern void _local_bh_enable(void);
>> >@@ -25,11 +29,13 @@ extern void __local_bh_enable_ip(unsigned long ip, unsigned int cnt);
>> > 
>> > static inline void local_bh_enable_ip(unsigned long ip)
>> > {
>> >+	lock_map_release(&bh_lock_map);
>> > 	__local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET);
>> > }
>> > 
>> > static inline void local_bh_enable(void)
>> > {
>> >+	lock_map_release(&bh_lock_map);
>> > 	__local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
>> > }
>> > 
>> >diff --git a/kernel/softirq.c b/kernel/softirq.c
>> >index 8b41bd13cc3d..17d9bf6e0caf 100644
>> >--- a/kernel/softirq.c
>> >+++ b/kernel/softirq.c
>> >@@ -1066,3 +1066,13 @@ unsigned int __weak arch_dynirq_lower_bound(unsigned int from)
>> > {
>> > 	return from;
>> > }
>> >+
>> >+static struct lock_class_key bh_lock_key;
>> >+struct lockdep_map bh_lock_map = {
>> >+	.name = "local_bh",
>> >+	.key = &bh_lock_key,
>> >+	.wait_type_outer = LD_WAIT_FREE,
>> >+	.wait_type_inner = LD_WAIT_CONFIG, /* PREEMPT_RT makes BH preemptible. */
>> >+	.lock_type = LD_LOCK_PERCPU,
>> >+};
>> >+EXPORT_SYMBOL_GPL(bh_lock_map);
>> 
>> Self-tests are now all passing. Thanks!
>> It looks good to me.
>
>Good! Do you want to continue working on this based on the above
>changes? If so, feel free.

Oh yes, for sure!!
I'll send the next version based on it with the Sebastian's earlier
suggestion included [0].

>Regards,
>Boqun

Sincerely,
Ryo Takakura

[0] https://lore.kernel.org/lkml/20241220071554.YAD157bS@linutronix.de/