Re: Q: lockdep_assert_held_read() after downgrade_write()
Peter Zijlstra: > Does something like the below work better? The annotation in > downgrade_write() would look something like: > > + lock_downgrade(&sem->dep_map, 1, _RET_IP_); > > Not even compile tested and lacks the !LOCKDEP build bits. Thanks for the patch. It seems working expectedly. I began writing a similar patch locally with minor consolidations by adding a new function or two. I will send a patch series. Please review and merge them into v4.10. If you don't like the patch, especially the new function name, feel free to change it. I don't know whether !LOCKDEP build bits are necessary or not. J. R. Okajima
Re: Q: lockdep_assert_held_read() after downgrade_write()
On Wed, Feb 01, 2017 at 12:40:03AM +0900, J. R. Okajima wrote: > Now allow me going on the second test (based upon Peter's patch) > > - two rwsem, rwA and rwB. > - the locking order is rwA first, and then rwB. > - good case > down_read(rwA) > down_read(rwB) > up_read(rwB) > up_read(rwA) > > down_write(rwA) > down_write(rwB) > up_write(rwB) > up_write(rwA) > > - questionable case > down_write(rwA) > down_write(rwB) > downgrade_write(rwA) > downgrade_write(rwB) > up_read(rwB) > up_read(rwA) > > These two downgrade_write() have their strict order? If so, what is > that? > Do the added two lines > + rwsem_release(&sem->dep_map, 1, _RET_IP_); > + rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_); > produce a traditional AB-BA deadlock warning, don't they? Blergh, yes, because we do a full release. Does something like the below work better? The annotation in downgrade_write() would look something like: + lock_downgrade(&sem->dep_map, 1, _RET_IP_); Not even compile tested and lacks the !LOCKDEP build bits. --- diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index 1e327bb..76cf149 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -361,6 +361,8 @@ static inline void lock_set_subclass(struct lockdep_map *lock, lock_set_class(lock, lock->name, lock->key, subclass, ip); } +extern void lock_downgrade(struct lockdep_map *lock, int read, unsigned long ip); + extern void lockdep_set_current_reclaim_state(gfp_t gfp_mask); extern void lockdep_clear_current_reclaim_state(void); extern void lockdep_trace_alloc(gfp_t mask); diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 7c38f8f..88517b6 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -3488,6 +3488,63 @@ __lock_set_class(struct lockdep_map *lock, const char *name, return 1; } +static int __lock_downgrade(struct lockdep_map *lock, int read, unsigned long ip) +{ + struct task_struct *curr = current; + struct held_lock *hlock, *prev_hlock; + struct lock_class *class; + unsigned int depth; + int i; + + depth = curr->lockdep_depth; + /* +* This function is about (re)setting the class of a held lock, +* yet we're not actually holding any locks. Naughty user! +*/ + if (DEBUG_LOCKS_WARN_ON(!depth)) + return 0; + + prev_hlock = NULL; + for (i = depth-1; i >= 0; i--) { + hlock = curr->held_locks + i; + /* +* We must not cross into another context: +*/ + if (prev_hlock && prev_hlock->irq_context != hlock->irq_context) + break; + if (match_held_lock(hlock, lock)) + goto found_it; + prev_hlock = hlock; + } + return print_unlock_imbalance_bug(curr, lock, ip); + +found_it: + curr->lockdep_depth = i; + curr->curr_chain_key = hlock->prev_chain_key; + + WARN(hlock->read, "downgrading a read lock"); + hlock->read = read; + hlock->acquire_ip = ip; + + for (; i < depth; i++) { + hlock = curr->held_locks + i; + if (!__lock_acquire(hlock->instance, + hlock_class(hlock)->subclass, hlock->trylock, + hlock->read, hlock->check, hlock->hardirqs_off, + hlock->nest_lock, hlock->acquire_ip, + hlock->references, hlock->pin_count)) + return 0; + } + + /* +* I took it apart and put it back together again, except now I have +* these 'spare' parts.. where shall I put them. +*/ + if (DEBUG_LOCKS_WARN_ON(curr->lockdep_depth != depth)) + return 0; + return 1; +} + /* * Remove the lock to the list of currently held locks - this gets * called on mutex_unlock()/spin_unlock*() (or on a failed @@ -3732,6 +3789,23 @@ void lock_set_class(struct lockdep_map *lock, const char *name, } EXPORT_SYMBOL_GPL(lock_set_class); +void lock_downgrade(struct lockdep_map *lock, int read, unsigned long ip) +{ + unsigned long flags; + + if (unlikely(current->lockdep_recursion)) + return; + + raw_local_irq_save(flags); + current->lockdep_recursion = 1; + check_flags(flags); + if (__lock_downgrade(lock, read, ip)) + check_chain_key(current); + current->lockdep_recursion = 0; + raw_local_irq_restore(flags); +} +EXPORT_SYMBOL_GPL(lock_downgrade); + /* * We are not always called with irqs disabled - do that here, * and also avoid lockdep recursion:
Re: Q: lockdep_assert_held_read() after downgrade_write()
Jens Axboe: > I don't think you understand how it works. downgrade_write() turns a write > lock into read held. To make that last sequence valid, you'd need: > > down_write(&rw); > downgrade_write(&rw); > lockdep_assert_held_read(&rw) > up_read(&rw); > > or just not drop up_write() from the last section. Arg... It is my bonehead mistake that I inserted up_write() before downgrade_write(). Sorry about that. Fortunately Peter Zijlstra reviewed downgrade_write() and sent a patch. Thank you, it passed my first test. Now allow me going on the second test (based upon Peter's patch) - two rwsem, rwA and rwB. - the locking order is rwA first, and then rwB. - good case down_read(rwA) down_read(rwB) up_read(rwB) up_read(rwA) down_write(rwA) down_write(rwB) up_write(rwB) up_write(rwA) - questionable case down_write(rwA) down_write(rwB) downgrade_write(rwA) downgrade_write(rwB) up_read(rwB) up_read(rwA) These two downgrade_write() have their strict order? If so, what is that? Do the added two lines + rwsem_release(&sem->dep_map, 1, _RET_IP_); + rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_); produce a traditional AB-BA deadlock warning, don't they? J. R. Okajima
Re: Q: lockdep_assert_held_read() after downgrade_write()
On 01/31/2017 06:25 AM, Peter Zijlstra wrote: > On Tue, Jan 31, 2017 at 11:36:20AM +0100, Peter Zijlstra wrote: >> On Mon, Jan 30, 2017 at 02:30:45PM -0700, Jens Axboe wrote: >>> I don't think you understand how it works. downgrade_write() turns a write >>> lock into read held. To make that last sequence valid, you'd need: >> Correct, and I'm surprised that didn't explode in different ways. >> >>> down_write(&rw); >>> downgrade_write(&rw); >>> lockdep_assert_held_read(&rw) >>> up_read(&rw); >>> >>> or just not drop up_write() from the last section. >> Right, but also, there seems to be a missing lockdep annotation to make >> that work. That is, downgrade_write() doesn't have a lockdep annotation, >> so it (lockdep) will still think its a write lock. >> >> >> Let me try and fix both issues. > Something like so I suppose,... completely untested. > > There could be a good reason for the current lockdep behaviour, but I > cannot remember. > > --- > diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c > index 45ba475d4be3..dfa9e40f83d5 100644 > --- a/kernel/locking/rwsem.c > +++ b/kernel/locking/rwsem.c > @@ -123,10 +123,9 @@ EXPORT_SYMBOL(up_write); > */ > void downgrade_write(struct rw_semaphore *sem) > { > - /* > - * lockdep: a downgraded write will live on as a write > - * dependency. > - */ > + rwsem_release(&sem->dep_map, 1, _RET_IP_); > + rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_); > + > rwsem_set_reader_owned(sem); > __downgrade_write(sem); > } > diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h > index a699f4048ba1..3bd584c81b0b 100644 > --- a/kernel/locking/rwsem.h > +++ b/kernel/locking/rwsem.h > @@ -40,8 +40,10 @@ static inline void rwsem_set_reader_owned(struct > rw_semaphore *sem) >* do a write to the rwsem cacheline when it is really necessary >* to minimize cacheline contention. >*/ > - if (sem->owner != RWSEM_READER_OWNED) > + if (sem->owner != RWSEM_READER_OWNED) { > + WARN_ON_ONCE(sem->owner != current); > WRITE_ONCE(sem->owner, RWSEM_READER_OWNED); > + } > } > > static inline bool rwsem_owner_is_writer(struct task_struct *owner) I don't think you can do a WARN_ON_ONCE() check for sem->owner != current here. If the rwsem starts from an unlock state, sem->owner will be NULL and an incorrect warning message will be printed. Cheers, Longman
Re: Q: lockdep_assert_held_read() after downgrade_write()
On Tue, Jan 31, 2017 at 09:23:08AM -0500, Waiman Long wrote: > On 01/31/2017 06:25 AM, Peter Zijlstra wrote: > > @@ -40,8 +40,10 @@ static inline void rwsem_set_reader_owned(struct > > rw_semaphore *sem) > > * do a write to the rwsem cacheline when it is really necessary > > * to minimize cacheline contention. > > */ > > - if (sem->owner != RWSEM_READER_OWNED) > > + if (sem->owner != RWSEM_READER_OWNED) { > > + WARN_ON_ONCE(sem->owner != current); > > WRITE_ONCE(sem->owner, RWSEM_READER_OWNED); > > + } > > } > > > > static inline bool rwsem_owner_is_writer(struct task_struct *owner) > > I don't think you can do a WARN_ON_ONCE() check for sem->owner != > current here. If the rwsem starts from an unlock state, sem->owner will > be NULL and an incorrect warning message will be printed. Argh, I only looked at the downgrade_write() user and forgot to look if it was used elsewhere.
Re: Q: lockdep_assert_held_read() after downgrade_write()
On Tue, Jan 31, 2017 at 11:36:20AM +0100, Peter Zijlstra wrote: > On Mon, Jan 30, 2017 at 02:30:45PM -0700, Jens Axboe wrote: > > I don't think you understand how it works. downgrade_write() turns a write > > lock into read held. To make that last sequence valid, you'd need: > > Correct, and I'm surprised that didn't explode in different ways. > > > > > down_write(&rw); > > downgrade_write(&rw); > > lockdep_assert_held_read(&rw) > > up_read(&rw); > > > > or just not drop up_write() from the last section. > > Right, but also, there seems to be a missing lockdep annotation to make > that work. That is, downgrade_write() doesn't have a lockdep annotation, > so it (lockdep) will still think its a write lock. > > > Let me try and fix both issues. Something like so I suppose,... completely untested. There could be a good reason for the current lockdep behaviour, but I cannot remember. --- diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c index 45ba475d4be3..dfa9e40f83d5 100644 --- a/kernel/locking/rwsem.c +++ b/kernel/locking/rwsem.c @@ -123,10 +123,9 @@ EXPORT_SYMBOL(up_write); */ void downgrade_write(struct rw_semaphore *sem) { - /* -* lockdep: a downgraded write will live on as a write -* dependency. -*/ + rwsem_release(&sem->dep_map, 1, _RET_IP_); + rwsem_acquire_read(&sem->dep_map, 0, 0, _RET_IP_); + rwsem_set_reader_owned(sem); __downgrade_write(sem); } diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h index a699f4048ba1..3bd584c81b0b 100644 --- a/kernel/locking/rwsem.h +++ b/kernel/locking/rwsem.h @@ -40,8 +40,10 @@ static inline void rwsem_set_reader_owned(struct rw_semaphore *sem) * do a write to the rwsem cacheline when it is really necessary * to minimize cacheline contention. */ - if (sem->owner != RWSEM_READER_OWNED) + if (sem->owner != RWSEM_READER_OWNED) { + WARN_ON_ONCE(sem->owner != current); WRITE_ONCE(sem->owner, RWSEM_READER_OWNED); + } } static inline bool rwsem_owner_is_writer(struct task_struct *owner)
Re: Q: lockdep_assert_held_read() after downgrade_write()
On Mon, Jan 30, 2017 at 02:30:45PM -0700, Jens Axboe wrote: > On 01/30/2017 02:25 PM, J. R. Okajima wrote: > > Peter Zijlstra, > > > > May I ask you a question? > > v4.10-rc1 got a commit > > f831948 2016-11-30 locking/lockdep: Provide a type check for > > lock_is_held > > I've tested a little and lockdep splat a stack trace. > > > > { > > DECLARE_RWSEM(rw); > > static struct lock_class_key key; > > lockdep_set_class(&rw, &key); > > > > down_read(&rw); > > lockdep_assert_held_read(&rw); > > up_read(&rw); > > > > down_write(&rw); > > lockdep_assert_held_exclusive(&rw); > > up_write(&rw); > > > > downgrade_write(&rw); > > lockdep_assert_held_read(&rw); <-- here > > up_read(&rw); > > } > > > > I was expecting that lockdep_assert_held_read() splat nothing after > > downgrade_write(). Is this warning an intentional behaviour? > > > > Also the final up_read() gives me a warning too. It is produced at > > lockdep.c:3514:lock_release(): DEBUG_LOCKS_WARN_ON(depth <= 0) > > I don't think you understand how it works. downgrade_write() turns a write > lock into read held. To make that last sequence valid, you'd need: Correct, and I'm surprised that didn't explode in different ways. > > down_write(&rw); > downgrade_write(&rw); > lockdep_assert_held_read(&rw) > up_read(&rw); > > or just not drop up_write() from the last section. Right, but also, there seems to be a missing lockdep annotation to make that work. That is, downgrade_write() doesn't have a lockdep annotation, so it (lockdep) will still think its a write lock. Let me try and fix both issues.
Re: Q: lockdep_assert_held_read() after downgrade_write()
On 01/30/2017 02:25 PM, J. R. Okajima wrote: > Peter Zijlstra, > > May I ask you a question? > v4.10-rc1 got a commit > f831948 2016-11-30 locking/lockdep: Provide a type check for > lock_is_held > I've tested a little and lockdep splat a stack trace. > > { > DECLARE_RWSEM(rw); > static struct lock_class_key key; > lockdep_set_class(&rw, &key); > > down_read(&rw); > lockdep_assert_held_read(&rw); > up_read(&rw); > > down_write(&rw); > lockdep_assert_held_exclusive(&rw); > up_write(&rw); > > downgrade_write(&rw); > lockdep_assert_held_read(&rw); <-- here > up_read(&rw); > } > > I was expecting that lockdep_assert_held_read() splat nothing after > downgrade_write(). Is this warning an intentional behaviour? > > Also the final up_read() gives me a warning too. It is produced at > lockdep.c:3514:lock_release(): DEBUG_LOCKS_WARN_ON(depth <= 0) I don't think you understand how it works. downgrade_write() turns a write lock into read held. To make that last sequence valid, you'd need: down_write(&rw); downgrade_write(&rw); lockdep_assert_held_read(&rw) up_read(&rw); or just not drop up_write() from the last section. -- Jens Axboe
Q: lockdep_assert_held_read() after downgrade_write()
Peter Zijlstra, May I ask you a question? v4.10-rc1 got a commit f831948 2016-11-30 locking/lockdep: Provide a type check for lock_is_held I've tested a little and lockdep splat a stack trace. { DECLARE_RWSEM(rw); static struct lock_class_key key; lockdep_set_class(&rw, &key); down_read(&rw); lockdep_assert_held_read(&rw); up_read(&rw); down_write(&rw); lockdep_assert_held_exclusive(&rw); up_write(&rw); downgrade_write(&rw); lockdep_assert_held_read(&rw); <-- here up_read(&rw); } I was expecting that lockdep_assert_held_read() splat nothing after downgrade_write(). Is this warning an intentional behaviour? Also the final up_read() gives me a warning too. It is produced at lockdep.c:3514:lock_release(): DEBUG_LOCKS_WARN_ON(depth <= 0) As an additional information, I increased some lockdep constants. Do you think this is related? include/linux/lockdep.h +#define MAX_LOCKDEP_SUBCLASSES (8UL + 4) +#define MAX_LOCKDEP_KEYS_BITS (13 + 3) kernel/locking/lockdep_internals.h +#define MAX_LOCKDEP_ENTRIES(32768UL << 5) +#define MAX_LOCKDEP_CHAINS_BITS(16 + 5) +#define MAX_STACK_TRACE_ENTRIES(524288UL << 5) J. R. Okajima