Messages in this thread | | | Subject | Re: [PATCH] locking/lockdep: Add debug_locks check in __lock_downgrade() | From | Tetsuo Handa <> | Date | Mon, 14 Jan 2019 22:23:10 +0900 |
| |
On 2019/01/10 19:21, Dmitry Vyukov wrote: >> @@ -3535,6 +3535,9 @@ static int __lock_downgrade(struct lockdep_map *lock, unsigned long ip) >> unsigned int depth; >> int i; >> >> + if (unlikely(!debug_locks)) >> + return 0; >> + > > Are we sure this resolves the problem rather than makes the > inconsistency window smaller?
As far as I know, this should resolve the problem.
> I don't understand all surrounding code, but looking just at this > function it looks like it may just pepper over the problem. Say, we > pass this check when lockdep was still turned on. Then this thread is > preempted for some time (e.g. a virtual CPU), then another thread > started reporting a warning, turned lockdep off, some information > wasn't collected, and this this task resumes and reports a false > warning.
What this function checks is whether current thread is holding rw_semaphore for write. Since the information of held locks are per "struct task_struct" record, if lockdep is still enabled as of entry of this function, there must be a lockdep record that current thread is holding rw_semaphore for write if current thread is actually holding rw_semaphore for write. Therefore, preemption/interrupts can't erase the lockdep record that current thread is holding rw_semaphore, even if lockdep is turned off after passing this check.
> Or we are holding the mutex here, and the fact that we are holding it > ensures that no other task will take it and no information will be > lost? > Quite a tricky moment, perhaps deserves a comment.
I think many other functions check debug_locks upon entry of functions.
> >> depth = curr->lockdep_depth; >> /* >> * This function is about (re)setting the class of a held lock,
| |