| Message ID | 20240124115938.80132-15-byungchul@sk.com |
|---|---|
| State | New |
| Headers | show |
| Series | DEPT(Dependency Tracker) | expand |
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
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
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
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
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
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)
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(-)