diff mbox series

[v11,14/26] locking/lockdep, cpu/hotplus: Use a weaker annotation in AP thread

Message ID 20240124115938.80132-15-byungchul@sk.com (mailing list archive)
State New, archived
Headers show
Series DEPT(Dependency Tracker) | expand

Commit Message

Byungchul Park Jan. 24, 2024, 11:59 a.m. UTC
cb92173d1f0 ("locking/lockdep, cpu/hotplug: Annotate AP thread") was
introduced to make lockdep_assert_cpus_held() work in AP thread.

However, the annotation is too strong for that purpose. We don't have to
use more than try lock annotation for that.

Furthermore, now that Dept was introduced, false positive alarms was
reported by that. Replaced it with try lock annotation.

Signed-off-by: Byungchul Park <byungchul@sk.com>
---
 kernel/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Thomas Gleixner Jan. 26, 2024, 5:30 p.m. UTC | #1
On Wed, Jan 24 2024 at 20:59, Byungchul Park wrote:

Why is lockdep in the subsystem prefix here? You are changing the CPU
hotplug (not hotplus) code, right?

> cb92173d1f0 ("locking/lockdep, cpu/hotplug: Annotate AP thread") was
> introduced to make lockdep_assert_cpus_held() work in AP thread.
>
> However, the annotation is too strong for that purpose. We don't have to
> use more than try lock annotation for that.

This lacks a proper explanation why this is too strong.

> Furthermore, now that Dept was introduced, false positive alarms was
> reported by that. Replaced it with try lock annotation.

I still have zero idea what this is about.

Thanks,

        tglx
Byungchul Park Jan. 29, 2024, 4:20 a.m. UTC | #2
On Fri, Jan 26, 2024 at 06:30:02PM +0100, Thomas Gleixner wrote:
> On Wed, Jan 24 2024 at 20:59, Byungchul Park wrote:
> 
> Why is lockdep in the subsystem prefix here? You are changing the CPU
> hotplug (not hotplus) code, right?

I will fix the typo ;( Thank you.

I referred to the commit cb92173d1f047. I will remove the prefix if the
way is more desirable.

> > cb92173d1f0 ("locking/lockdep, cpu/hotplug: Annotate AP thread") was
> > introduced to make lockdep_assert_cpus_held() work in AP thread.
> >
> > However, the annotation is too strong for that purpose. We don't have to
> > use more than try lock annotation for that.
> 
> This lacks a proper explanation why this is too strong.

rwsem_acquire() implies:

   1. might be a waiter on contention of the lock.
   2. enter to the critical section of the lock.

All we need in here is to act 2, not 1. That's why I suggested trylock
version of annotation for that purpose.

Now that dept partially replies on lockdep annotaions for the waiters
and events, dept is interpeting rwsem_acquire() as a potential waiter
and reports a deadlock by the wait.

Of course, the first priority should be not to change the current
behavior. I think the change from non-trylock to trylock for the
annotation won't. Or am I missing something?

	Byungchul

> > Furthermore, now that Dept was introduced, false positive alarms was
> > reported by that. Replaced it with try lock annotation.
> 
> I still have zero idea what this is about.
> 
> Thanks,
> 
>         tglx
Byungchul Park Jan. 30, 2024, 2:58 a.m. UTC | #3
On Fri, Jan 26, 2024 at 06:30:02PM +0100, Thomas Gleixner wrote:
> On Wed, Jan 24 2024 at 20:59, Byungchul Park wrote:
> 
> Why is lockdep in the subsystem prefix here? You are changing the CPU
> hotplug (not hotplus) code, right?
> 
> > cb92173d1f0 ("locking/lockdep, cpu/hotplug: Annotate AP thread") was
> > introduced to make lockdep_assert_cpus_held() work in AP thread.
> >
> > However, the annotation is too strong for that purpose. We don't have to
> > use more than try lock annotation for that.
> 
> This lacks a proper explanation why this is too strong.
> 
> > Furthermore, now that Dept was introduced, false positive alarms was
> > reported by that. Replaced it with try lock annotation.
> 
> I still have zero idea what this is about.

1. can track PG_locked that is a potential deadlock trigger.

   https://lore.kernel.org/lkml/1674268856-31807-1-git-send-email-byungchul.park@lge.com/

2. can track any waits/events e.g. wait_for_xxx(), dma fence and so on.

3. easy to annotate using dept_wait() on waits, dept_event() on events.

4. track read lock better way instead of the ugly way, by assinging wait
   or event annotations onto read lock and write lock. For instrance, a
   read lock is annotated as a potential waiter for its write unlock,
   and a write lock is annotated as a potential waiter for either write
   unlock or read unlock.

I'd like to remove unnecessary complexity on deadlock detection and add
additional functionality by making it do what the type of tool exactly
should do.

	Byungchul

> Thanks,
> 
>         tglx
Thomas Gleixner Feb. 12, 2024, 3:16 p.m. UTC | #4
On Tue, Jan 30 2024 at 11:58, Byungchul Park wrote:
> On Fri, Jan 26, 2024 at 06:30:02PM +0100, Thomas Gleixner wrote:
>> On Wed, Jan 24 2024 at 20:59, Byungchul Park wrote:
>> 
>> Why is lockdep in the subsystem prefix here? You are changing the CPU
>> hotplug (not hotplus) code, right?
>> 
>> > cb92173d1f0 ("locking/lockdep, cpu/hotplug: Annotate AP thread") was
>> > introduced to make lockdep_assert_cpus_held() work in AP thread.
>> >
>> > However, the annotation is too strong for that purpose. We don't have to
>> > use more than try lock annotation for that.
>> 
>> This lacks a proper explanation why this is too strong.
>> 
>> > Furthermore, now that Dept was introduced, false positive alarms was
>> > reported by that. Replaced it with try lock annotation.
>> 
>> I still have zero idea what this is about.
>
> 1. can track PG_locked that is a potential deadlock trigger.
>
>    https://lore.kernel.org/lkml/1674268856-31807-1-git-send-email-byungchul.park@lge.com/

Sure, but that wants to be explicitely explained in the changelog and
not with a link. 'Now that Dept was introduced ...' is not an
explanation.

Thanks,

        tglx
Byungchul Park Feb. 13, 2024, 1:18 a.m. UTC | #5
On Mon, Feb 12, 2024 at 04:16:41PM +0100, Thomas Gleixner wrote:
> On Tue, Jan 30 2024 at 11:58, Byungchul Park wrote:
> > On Fri, Jan 26, 2024 at 06:30:02PM +0100, Thomas Gleixner wrote:
> >> On Wed, Jan 24 2024 at 20:59, Byungchul Park wrote:
> >> 
> >> Why is lockdep in the subsystem prefix here? You are changing the CPU
> >> hotplug (not hotplus) code, right?
> >> 
> >> > cb92173d1f0 ("locking/lockdep, cpu/hotplug: Annotate AP thread") was
> >> > introduced to make lockdep_assert_cpus_held() work in AP thread.
> >> >
> >> > However, the annotation is too strong for that purpose. We don't have to
> >> > use more than try lock annotation for that.
> >> 
> >> This lacks a proper explanation why this is too strong.
> >> 
> >> > Furthermore, now that Dept was introduced, false positive alarms was
> >> > reported by that. Replaced it with try lock annotation.
> >> 
> >> I still have zero idea what this is about.
> >
> > 1. can track PG_locked that is a potential deadlock trigger.
> >
> >    https://lore.kernel.org/lkml/1674268856-31807-1-git-send-email-byungchul.park@lge.com/
> 
> Sure, but that wants to be explicitely explained in the changelog and
> not with a link. 'Now that Dept was introduced ...' is not an
> explanation.

Admit. I will fix it from the next spin. Thanks.

	Byungchul
Byungchul Park June 4, 2024, 3:10 a.m. UTC | #6
On Fri, Jan 26, 2024 at 06:30:02PM +0100, Thomas Gleixner wrote:
> > Furthermore, now that Dept was introduced, false positive alarms was
> > reported by that. Replaced it with try lock annotation.
> 
> I still have zero idea what this is about.

Lockdep is working on lock/unlock, while dept is working on wait/event.

Two are similar but strickly speaking, different in what to track.

	Byungchul
diff mbox series

Patch

diff --git a/kernel/cpu.c b/kernel/cpu.c
index a86972a91991..b708989f789f 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -535,7 +535,7 @@  int lockdep_is_cpus_held(void)
 
 static void lockdep_acquire_cpus_lock(void)
 {
-	rwsem_acquire(&cpu_hotplug_lock.dep_map, 0, 0, _THIS_IP_);
+	rwsem_acquire(&cpu_hotplug_lock.dep_map, 0, 1, _THIS_IP_);
 }
 
 static void lockdep_release_cpus_lock(void)