Hi,
This series enables deadlock detection for srcu_read_lock() vs synchronize_srcu().
Again, my first time helping prepare PR, so please take a careful look and yell at me if there is something wrong. Thanks a lot!
You will also be able to find the series at:
https://github/fbq/linux rcu/lockdep.2023.03.12a
top commit is:
24390de55773
List of changes:
Boqun Feng (4): locking/lockdep: Introduce lock_sync() rcu: Annotate SRCU's update-side lockdep dependencies locking: Reduce the number of locks in ww_mutex stress tests locking/lockdep: Improve the deadlock scenario print for sync and read lock
Paul E. McKenney (3): rcutorture: Add SRCU deadlock scenarios rcutorture: Add RCU Tasks Trace and SRCU deadlock scenarios rcutorture: Add srcu_lockdep.sh
include/linux/lockdep.h | 8 +- include/linux/srcu.h | 34 +++- kernel/locking/lockdep.c | 64 +++++- kernel/locking/test-ww_mutex.c | 2 +- kernel/rcu/rcutorture.c | 185 ++++++++++++++++++ kernel/rcu/srcutiny.c | 2 + kernel/rcu/srcutree.c | 2 + .../selftests/rcutorture/bin/srcu_lockdep.sh | 73 +++++++ 8 files changed, 359 insertions(+), 11 deletions(-) create mode 100755 tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
Currently, functions like synchronize_srcu() do not have lockdep annotations resembling those of other write-side locking primitives. Such annotations might look as follows:
lock_acquire(); lock_release();
Such annotations would tell lockdep that synchronize_srcu() acts like an empty critical section that waits for other (read-side) critical sections to finish. This would definitely catch some deadlock, but as pointed out by Paul Mckenney [1], this could also introduce false positives because of irq-safe/unsafe detection. Of course, there are tricks could help with this:
might_sleep(); // Existing statement in __synchronize_srcu(). if (IS_ENABLED(CONFIG_PROVE_LOCKING)) { local_irq_disable(); lock_acquire(); lock_release(); local_irq_enable(); }
But it would be better for lockdep to provide a separate annonation for functions like synchronize_srcu(), so that people won't need to repeat the ugly tricks above.
Therefore introduce lock_sync(), which is simply an lock+unlock pair with no irq safe/unsafe deadlock check. This works because the to-be-annontated functions do not create real critical sections, and there is therefore no way that irq can create extra dependencies.
[1]: https://lore.kernel.org/lkml/20180412021233.ewncg5jjuzjw3x62@tardis/
Signed-off-by: Boqun Feng boqun.feng@gmail.com Acked-by: Waiman Long longman@redhat.com Signed-off-by: Paul E. McKenney paulmck@kernel.org Signed-off-by: Boqun Feng boqun.feng@gmail.com --- include/linux/lockdep.h | 5 +++++ kernel/locking/lockdep.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index 1023f349af71..14d9dbedc6c1 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -268,6 +268,10 @@ extern void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
extern void lock_release(struct lockdep_map *lock, unsigned long ip);
+extern void lock_sync(struct lockdep_map *lock, unsigned int subclass, + int read, int check, struct lockdep_map *nest_lock, + unsigned long ip); + /* lock_is_held_type() returns */ #define LOCK_STATE_UNKNOWN -1 #define LOCK_STATE_NOT_HELD 0 @@ -554,6 +558,7 @@ do { \ #define lock_map_acquire_read(l) lock_acquire_shared_recursive(l, 0, 0, NULL, _THIS_IP_) #define lock_map_acquire_tryread(l) lock_acquire_shared_recursive(l, 0, 1, NULL, _THIS_IP_) #define lock_map_release(l) lock_release(l, _THIS_IP_) +#define lock_map_sync(l) lock_sync(l, 0, 0, 1, NULL, _THIS_IP_)
#ifdef CONFIG_PROVE_LOCKING # define might_lock(lock) \ diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 50d4863974e7..36430cf8e407 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -5693,6 +5693,40 @@ void lock_release(struct lockdep_map *lock, unsigned long ip) } EXPORT_SYMBOL_GPL(lock_release);
+/* + * lock_sync() - A special annotation for synchronize_{s,}rcu()-like API. + * + * No actual critical section is created by the APIs annotated with this: these + * APIs are used to wait for one or multiple critical sections (on other CPUs + * or threads), and it means that calling these APIs inside these critical + * sections is potential deadlock. + * + * This annotation acts as an acqurie+release anontation pair with hardirqoff + * being 1. Since there's no critical section, no interrupt can create extra + * dependencies "inside" the annotation, hardirqoff == 1 allows us to avoid + * false positives. + */ +void lock_sync(struct lockdep_map *lock, unsigned subclass, int read, + int check, struct lockdep_map *nest_lock, unsigned long ip) +{ + unsigned long flags; + + if (unlikely(!lockdep_enabled())) + return; + + raw_local_irq_save(flags); + check_flags(flags); + + lockdep_recursion_inc(); + __lock_acquire(lock, subclass, 0, read, check, 1, nest_lock, ip, 0, 0); + + if (__lock_release(lock, ip)) + check_chain_key(current); + lockdep_recursion_finish(); + raw_local_irq_restore(flags); +} +EXPORT_SYMBOL_GPL(lock_sync); + noinstr int lock_is_held_type(const struct lockdep_map *lock, int read) { unsigned long flags;
On Thu, 16 Mar 2023, Boqun Feng wrote:
+/*
- lock_sync() - A special annotation for synchronize_{s,}rcu()-like API.
- No actual critical section is created by the APIs annotated with this: these
- APIs are used to wait for one or multiple critical sections (on other CPUs
- or threads), and it means that calling these APIs inside these critical
- sections is potential deadlock.
- This annotation acts as an acqurie+release anontation pair with hardirqoff
^acquire
On Mon, Mar 20, 2023 at 10:06:14AM -0700, Davidlohr Bueso wrote:
On Thu, 16 Mar 2023, Boqun Feng wrote:
+/*
- lock_sync() - A special annotation for synchronize_{s,}rcu()-like API.
- No actual critical section is created by the APIs annotated with this: these
- APIs are used to wait for one or multiple critical sections (on other CPUs
- or threads), and it means that calling these APIs inside these critical
- sections is potential deadlock.
- This annotation acts as an acqurie+release anontation pair with hardirqoff
^acquire
Good eye! Applied locally.
Regards, Boqun
Although all flavors of RCU readers are annotated correctly with lockdep as recursive read locks, they do not set the lock_acquire 'check' parameter. This means that RCU read locks are not added to the lockdep dependency graph, which in turn means that lockdep cannot detect RCU-based deadlocks. This is not a problem for RCU flavors having atomic read-side critical sections because context-based annotations can catch these deadlocks, see for example the RCU_LOCKDEP_WARN() statement in synchronize_rcu(). But context-based annotations are not helpful for sleepable RCU, especially given that it is perfectly legal to do synchronize_srcu(&srcu1) within an srcu_read_lock(&srcu2).
However, we can detect SRCU-based by: (1) Making srcu_read_lock() a 'check'ed recursive read lock and (2) Making synchronize_srcu() a empty write lock critical section. Even better, with the newly introduced lock_sync(), we can avoid false positives about irq-unsafe/safe. This commit therefore makes it so.
Note that NMI-safe SRCU read side critical sections are currently not annotated, but might be annotated in the future.
Signed-off-by: Boqun Feng boqun.feng@gmail.com Signed-off-by: Paul E. McKenney paulmck@kernel.org [ boqun: Add comments for annotation per Waiman's suggestion ] Signed-off-by: Boqun Feng boqun.feng@gmail.com --- include/linux/srcu.h | 34 ++++++++++++++++++++++++++++++++-- kernel/rcu/srcutiny.c | 2 ++ kernel/rcu/srcutree.c | 2 ++ 3 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/include/linux/srcu.h b/include/linux/srcu.h index 74796cd7e7a9..7a745169b79a 100644 --- a/include/linux/srcu.h +++ b/include/linux/srcu.h @@ -102,6 +102,32 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp) return lock_is_held(&ssp->dep_map); }
+/** + * Annotations provide deadlock detection for SRCU. + * + * Similar to other lockdep annotations, except there is an additional + * srcu_lock_sync(), which is basically an empty *write*-side critical section, + * see lock_sync() for more information. + */ + +/* Annotates a srcu_read_lock() */ +static inline void srcu_lock_acquire(struct lockdep_map *map) +{ + lock_map_acquire_read(map); +} + +/* Annotates a srcu_read_lock() */ +static inline void srcu_lock_release(struct lockdep_map *map) +{ + lock_map_release(map); +} + +/* Annotates a synchronize_srcu() */ +static inline void srcu_lock_sync(struct lockdep_map *map) +{ + lock_map_sync(map); +} + #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
static inline int srcu_read_lock_held(const struct srcu_struct *ssp) @@ -109,6 +135,10 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp) return 1; }
+#define srcu_lock_acquire(m) do { } while (0) +#define srcu_lock_release(m) do { } while (0) +#define srcu_lock_sync(m) do { } while (0) + #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
#define SRCU_NMI_UNKNOWN 0x0 @@ -182,7 +212,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp)
srcu_check_nmi_safety(ssp, false); retval = __srcu_read_lock(ssp); - rcu_lock_acquire(&(ssp)->dep_map); + srcu_lock_acquire(&(ssp)->dep_map); return retval; }
@@ -254,7 +284,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx) { WARN_ON_ONCE(idx & ~0x1); srcu_check_nmi_safety(ssp, false); - rcu_lock_release(&(ssp)->dep_map); + srcu_lock_release(&(ssp)->dep_map); __srcu_read_unlock(ssp, idx); }
diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c index b12fb0cec44d..336af24e0fe3 100644 --- a/kernel/rcu/srcutiny.c +++ b/kernel/rcu/srcutiny.c @@ -197,6 +197,8 @@ void synchronize_srcu(struct srcu_struct *ssp) { struct rcu_synchronize rs;
+ srcu_lock_sync(&ssp->dep_map); + RCU_LOCKDEP_WARN(lockdep_is_held(ssp) || lock_is_held(&rcu_bh_lock_map) || lock_is_held(&rcu_lock_map) || diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index ab4ee58af84b..c541b82646b6 100644 --- a/kernel/rcu/srcutree.c +++ b/kernel/rcu/srcutree.c @@ -1307,6 +1307,8 @@ static void __synchronize_srcu(struct srcu_struct *ssp, bool do_norm) { struct rcu_synchronize rcu;
+ srcu_lock_sync(&ssp->dep_map); + RCU_LOCKDEP_WARN(lockdep_is_held(ssp) || lock_is_held(&rcu_bh_lock_map) || lock_is_held(&rcu_lock_map) ||
The stress test in test_ww_mutex_init() uses 4095 locks since lockdep::reference has 12 bits, and since we are going to reduce it to 11 bits to support lock_sync(), and 2047 is still a reasonable number of the max nesting level for locks, so adjust the test.
Reported-by: kernel test robot oliver.sang@intel.com Link: https://lore.kernel.org/oe-lkp/202302011445.9d99dae2-oliver.sang@intel.com Signed-off-by: Boqun Feng boqun.feng@gmail.com --- kernel/locking/test-ww_mutex.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/locking/test-ww_mutex.c b/kernel/locking/test-ww_mutex.c index 29dc253d03af..93cca6e69860 100644 --- a/kernel/locking/test-ww_mutex.c +++ b/kernel/locking/test-ww_mutex.c @@ -659,7 +659,7 @@ static int __init test_ww_mutex_init(void) if (ret) return ret;
- ret = stress(4095, hweight32(STRESS_ALL)*ncpus, STRESS_ALL); + ret = stress(2047, hweight32(STRESS_ALL)*ncpus, STRESS_ALL); if (ret) return ret;
On Thu, Mar 16, 2023 at 08:13:35PM -0700, Boqun Feng wrote:
The stress test in test_ww_mutex_init() uses 4095 locks since lockdep::reference has 12 bits, and since we are going to reduce it to 11 bits to support lock_sync(), and 2047 is still a reasonable number of the max nesting level for locks, so adjust the test.
Reported-by: kernel test robot oliver.sang@intel.com Link: https://lore.kernel.org/oe-lkp/202302011445.9d99dae2-oliver.sang@intel.com Signed-off-by: Boqun Feng boqun.feng@gmail.com
Tested-by: Paul E. McKenney paulmck@kernel.org
kernel/locking/test-ww_mutex.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/locking/test-ww_mutex.c b/kernel/locking/test-ww_mutex.c index 29dc253d03af..93cca6e69860 100644 --- a/kernel/locking/test-ww_mutex.c +++ b/kernel/locking/test-ww_mutex.c @@ -659,7 +659,7 @@ static int __init test_ww_mutex_init(void) if (ret) return ret;
- ret = stress(4095, hweight32(STRESS_ALL)*ncpus, STRESS_ALL);
- ret = stress(2047, hweight32(STRESS_ALL)*ncpus, STRESS_ALL); if (ret) return ret;
2.39.2
On Fri, Mar 17, 2023 at 11:38:19AM -0700, Paul E. McKenney wrote:
On Thu, Mar 16, 2023 at 08:13:35PM -0700, Boqun Feng wrote:
The stress test in test_ww_mutex_init() uses 4095 locks since lockdep::reference has 12 bits, and since we are going to reduce it to 11 bits to support lock_sync(), and 2047 is still a reasonable number of the max nesting level for locks, so adjust the test.
Reported-by: kernel test robot oliver.sang@intel.com Link: https://lore.kernel.org/oe-lkp/202302011445.9d99dae2-oliver.sang@intel.com Signed-off-by: Boqun Feng boqun.feng@gmail.com
Tested-by: Paul E. McKenney paulmck@kernel.org
Applied, thanks!
Regards, Boqun
kernel/locking/test-ww_mutex.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/locking/test-ww_mutex.c b/kernel/locking/test-ww_mutex.c index 29dc253d03af..93cca6e69860 100644 --- a/kernel/locking/test-ww_mutex.c +++ b/kernel/locking/test-ww_mutex.c @@ -659,7 +659,7 @@ static int __init test_ww_mutex_init(void) if (ret) return ret;
- ret = stress(4095, hweight32(STRESS_ALL)*ncpus, STRESS_ALL);
- ret = stress(2047, hweight32(STRESS_ALL)*ncpus, STRESS_ALL); if (ret) return ret;
2.39.2
Lock scenario print is always a weak spot of lockdep splats. Improvement can be made if we rework the dependency search and the error printing.
However without touching the graph search, we can improve a little for the circular deadlock case, since we have the to-be-added lock dependency, and know whether these two locks are read/write/sync.
In order to know whether a held_lock is sync or not, a bit was "stolen" from ->references, which reduce our limit for the same lock class nesting from 2^12 to 2^11, and it should still be good enough.
Besides, since we now have bit in held_lock for sync, we don't need the "hardirqoffs being 1" trick, and also we can avoid the __lock_release() if we jump out of __lock_acquire() before the held_lock stored.
With these changes, a deadlock case evolved with read lock and sync gets a better print-out from:
[...] Possible unsafe locking scenario: [...] [...] CPU0 CPU1 [...] ---- ---- [...] lock(srcuA); [...] lock(srcuB); [...] lock(srcuA); [...] lock(srcuB);
to
[...] Possible unsafe locking scenario: [...] [...] CPU0 CPU1 [...] ---- ---- [...] rlock(srcuA); [...] lock(srcuB); [...] lock(srcuA); [...] sync(srcuB);
Signed-off-by: Boqun Feng boqun.feng@gmail.com Signed-off-by: Paul E. McKenney paulmck@kernel.org Signed-off-by: Boqun Feng boqun.feng@gmail.com --- include/linux/lockdep.h | 3 ++- kernel/locking/lockdep.c | 48 ++++++++++++++++++++++++++-------------- 2 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index 14d9dbedc6c1..b32256e9e944 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -134,7 +134,8 @@ struct held_lock { unsigned int read:2; /* see lock_acquire() comment */ unsigned int check:1; /* see lock_acquire() comment */ unsigned int hardirqs_off:1; - unsigned int references:12; /* 32 bits */ + unsigned int sync:1; + unsigned int references:11; /* 32 bits */ unsigned int pin_count; };
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 36430cf8e407..dcd1d5bfc1e0 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -1881,6 +1881,8 @@ print_circular_lock_scenario(struct held_lock *src, struct lock_class *source = hlock_class(src); struct lock_class *target = hlock_class(tgt); struct lock_class *parent = prt->class; + int src_read = src->read; + int tgt_read = tgt->read;
/* * A direct locking problem where unsafe_class lock is taken @@ -1908,7 +1910,10 @@ print_circular_lock_scenario(struct held_lock *src, printk(" Possible unsafe locking scenario:\n\n"); printk(" CPU0 CPU1\n"); printk(" ---- ----\n"); - printk(" lock("); + if (tgt_read != 0) + printk(" rlock("); + else + printk(" lock("); __print_lock_name(target); printk(KERN_CONT ");\n"); printk(" lock("); @@ -1917,7 +1922,12 @@ print_circular_lock_scenario(struct held_lock *src, printk(" lock("); __print_lock_name(target); printk(KERN_CONT ");\n"); - printk(" lock("); + if (src_read != 0) + printk(" rlock("); + else if (src->sync) + printk(" sync("); + else + printk(" lock("); __print_lock_name(source); printk(KERN_CONT ");\n"); printk("\n *** DEADLOCK ***\n\n"); @@ -4531,7 +4541,13 @@ mark_usage(struct task_struct *curr, struct held_lock *hlock, int check) return 0; } } - if (!hlock->hardirqs_off) { + + /* + * For lock_sync(), don't mark the ENABLED usage, since lock_sync() + * creates no critical section and no extra dependency can be introduced + * by interrupts + */ + if (!hlock->hardirqs_off && !hlock->sync) { if (hlock->read) { if (!mark_lock(curr, hlock, LOCK_ENABLED_HARDIRQ_READ)) @@ -4910,7 +4926,7 @@ static int __lock_is_held(const struct lockdep_map *lock, int read); static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass, int trylock, int read, int check, int hardirqs_off, struct lockdep_map *nest_lock, unsigned long ip, - int references, int pin_count) + int references, int pin_count, int sync) { struct task_struct *curr = current; struct lock_class *class = NULL; @@ -4961,7 +4977,8 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
class_idx = class - lock_classes;
- if (depth) { /* we're holding locks */ + if (depth && !sync) { + /* we're holding locks and the new held lock is not a sync */ hlock = curr->held_locks + depth - 1; if (hlock->class_idx == class_idx && nest_lock) { if (!references) @@ -4995,6 +5012,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass, hlock->trylock = trylock; hlock->read = read; hlock->check = check; + hlock->sync = !!sync; hlock->hardirqs_off = !!hardirqs_off; hlock->references = references; #ifdef CONFIG_LOCK_STAT @@ -5056,6 +5074,10 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass, if (!validate_chain(curr, hlock, chain_head, chain_key)) return 0;
+ /* For lock_sync(), we are done here since no actual critical section */ + if (hlock->sync) + return 1; + curr->curr_chain_key = chain_key; curr->lockdep_depth++; check_chain_key(curr); @@ -5197,7 +5219,7 @@ static int reacquire_held_locks(struct task_struct *curr, unsigned int depth, hlock->read, hlock->check, hlock->hardirqs_off, hlock->nest_lock, hlock->acquire_ip, - hlock->references, hlock->pin_count)) { + hlock->references, hlock->pin_count, 0)) { case 0: return 1; case 1: @@ -5667,7 +5689,7 @@ void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
lockdep_recursion_inc(); __lock_acquire(lock, subclass, trylock, read, check, - irqs_disabled_flags(flags), nest_lock, ip, 0, 0); + irqs_disabled_flags(flags), nest_lock, ip, 0, 0, 0); lockdep_recursion_finish(); raw_local_irq_restore(flags); } @@ -5700,11 +5722,6 @@ EXPORT_SYMBOL_GPL(lock_release); * APIs are used to wait for one or multiple critical sections (on other CPUs * or threads), and it means that calling these APIs inside these critical * sections is potential deadlock. - * - * This annotation acts as an acqurie+release anontation pair with hardirqoff - * being 1. Since there's no critical section, no interrupt can create extra - * dependencies "inside" the annotation, hardirqoff == 1 allows us to avoid - * false positives. */ void lock_sync(struct lockdep_map *lock, unsigned subclass, int read, int check, struct lockdep_map *nest_lock, unsigned long ip) @@ -5718,10 +5735,9 @@ void lock_sync(struct lockdep_map *lock, unsigned subclass, int read, check_flags(flags);
lockdep_recursion_inc(); - __lock_acquire(lock, subclass, 0, read, check, 1, nest_lock, ip, 0, 0); - - if (__lock_release(lock, ip)) - check_chain_key(current); + __lock_acquire(lock, subclass, 0, read, check, + irqs_disabled_flags(flags), nest_lock, ip, 0, 0, 1); + check_chain_key(current); lockdep_recursion_finish(); raw_local_irq_restore(flags); }
On Thu, Mar 16, 2023 at 08:13:36PM -0700, Boqun Feng wrote:
Lock scenario print is always a weak spot of lockdep splats. Improvement can be made if we rework the dependency search and the error printing.
However without touching the graph search, we can improve a little for the circular deadlock case, since we have the to-be-added lock dependency, and know whether these two locks are read/write/sync.
In order to know whether a held_lock is sync or not, a bit was "stolen" from ->references, which reduce our limit for the same lock class nesting from 2^12 to 2^11, and it should still be good enough.
Besides, since we now have bit in held_lock for sync, we don't need the "hardirqoffs being 1" trick, and also we can avoid the __lock_release() if we jump out of __lock_acquire() before the held_lock stored.
With these changes, a deadlock case evolved with read lock and sync gets a better print-out from:
[...] Possible unsafe locking scenario: [...] [...] CPU0 CPU1 [...] ---- ---- [...] lock(srcuA); [...] lock(srcuB); [...] lock(srcuA); [...] lock(srcuB);
to
[...] Possible unsafe locking scenario: [...] [...] CPU0 CPU1 [...] ---- ---- [...] rlock(srcuA); [...] lock(srcuB); [...] lock(srcuA); [...] sync(srcuB);
Signed-off-by: Boqun Feng boqun.feng@gmail.com Signed-off-by: Paul E. McKenney paulmck@kernel.org Signed-off-by: Boqun Feng boqun.feng@gmail.com
include/linux/lockdep.h | 3 ++- kernel/locking/lockdep.c | 48 ++++++++++++++++++++++++++-------------- 2 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index 14d9dbedc6c1..b32256e9e944 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -134,7 +134,8 @@ struct held_lock { unsigned int read:2; /* see lock_acquire() comment */ unsigned int check:1; /* see lock_acquire() comment */ unsigned int hardirqs_off:1;
- unsigned int references:12; /* 32 bits */
- unsigned int sync:1;
- unsigned int references:11; /* 32 bits */ unsigned int pin_count;
};
Yeah, I suppose we can do that -- another option is to steal some bits from pin_count, but whatever (references used to be 11 a long while ago, no problem going back to that).
Acked-by: Peter Zijlstra (Intel) peterz@infradead.org
On Mon, Mar 20, 2023 at 01:13:05PM +0100, Peter Zijlstra wrote:
On Thu, Mar 16, 2023 at 08:13:36PM -0700, Boqun Feng wrote:
Lock scenario print is always a weak spot of lockdep splats. Improvement can be made if we rework the dependency search and the error printing.
However without touching the graph search, we can improve a little for the circular deadlock case, since we have the to-be-added lock dependency, and know whether these two locks are read/write/sync.
In order to know whether a held_lock is sync or not, a bit was "stolen" from ->references, which reduce our limit for the same lock class nesting from 2^12 to 2^11, and it should still be good enough.
Besides, since we now have bit in held_lock for sync, we don't need the "hardirqoffs being 1" trick, and also we can avoid the __lock_release() if we jump out of __lock_acquire() before the held_lock stored.
With these changes, a deadlock case evolved with read lock and sync gets a better print-out from:
[...] Possible unsafe locking scenario: [...] [...] CPU0 CPU1 [...] ---- ---- [...] lock(srcuA); [...] lock(srcuB); [...] lock(srcuA); [...] lock(srcuB);
to
[...] Possible unsafe locking scenario: [...] [...] CPU0 CPU1 [...] ---- ---- [...] rlock(srcuA); [...] lock(srcuB); [...] lock(srcuA); [...] sync(srcuB);
Signed-off-by: Boqun Feng boqun.feng@gmail.com Signed-off-by: Paul E. McKenney paulmck@kernel.org Signed-off-by: Boqun Feng boqun.feng@gmail.com
include/linux/lockdep.h | 3 ++- kernel/locking/lockdep.c | 48 ++++++++++++++++++++++++++-------------- 2 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h index 14d9dbedc6c1..b32256e9e944 100644 --- a/include/linux/lockdep.h +++ b/include/linux/lockdep.h @@ -134,7 +134,8 @@ struct held_lock { unsigned int read:2; /* see lock_acquire() comment */ unsigned int check:1; /* see lock_acquire() comment */ unsigned int hardirqs_off:1;
- unsigned int references:12; /* 32 bits */
- unsigned int sync:1;
- unsigned int references:11; /* 32 bits */ unsigned int pin_count;
};
Yeah, I suppose we can do that -- another option is to steal some bits from pin_count, but whatever (references used to be 11 a long while ago, no problem going back to that).
Thanks!
Acked-by: Peter Zijlstra (Intel) peterz@infradead.org
Applied locally.
Regards, Boqun
From: "Paul E. McKenney" paulmck@kernel.org
In order to test the new SRCU-lockdep functionality, this commit adds an rcutorture.test_srcu_lockdep module parameter that, when non-zero, selects an SRCU deadlock scenario to execute. This parameter is a five-digit number formatted as DNNL, where "D" is 1 to force a deadlock and 0 to avoid doing so; "NN" is the test number, 0 for SRCU-based, 1 for SRCU/mutex-based, and 2 for SRCU/rwsem-based; and "L" is the number of steps in the deadlock cycle.
Note that rcutorture.test_srcu_lockdep=1 will also force a hard hang.
If a non-zero value of rcutorture.test_srcu_lockdep does not select a deadlock scenario, a console message is printed and testing continues.
[ paulmck: Apply kernel test robot feedback, add rwsem support. ] [ paulmck: Apply Dan Carpenter feedback. ]
Signed-off-by: Paul E. McKenney paulmck@kernel.org Signed-off-by: Boqun Feng boqun.feng@gmail.com --- kernel/rcu/rcutorture.c | 151 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 151 insertions(+)
diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c index 8e6c023212cb..80ff9a743d31 100644 --- a/kernel/rcu/rcutorture.c +++ b/kernel/rcu/rcutorture.c @@ -120,6 +120,7 @@ torture_param(int, test_boost, 1, "Test RCU prio boost: 0=no, 1=maybe, 2=yes."); torture_param(int, test_boost_duration, 4, "Duration of each boost test, seconds."); torture_param(int, test_boost_interval, 7, "Interval between boost tests, seconds."); torture_param(bool, test_no_idle_hz, true, "Test support for tickless idle CPUs"); +torture_param(int, test_srcu_lockdep, 0, "Test specified SRCU deadlock scenario."); torture_param(int, verbose, 1, "Enable verbose debugging printk()s");
static char *torture_type = "rcu"; @@ -3463,6 +3464,154 @@ static void rcutorture_sync(void) cur_ops->sync(); }
+static DEFINE_MUTEX(mut0); +static DEFINE_MUTEX(mut1); +static DEFINE_MUTEX(mut2); +static DEFINE_MUTEX(mut3); +static DEFINE_MUTEX(mut4); +static DEFINE_MUTEX(mut5); +static DEFINE_MUTEX(mut6); +static DEFINE_MUTEX(mut7); +static DEFINE_MUTEX(mut8); +static DEFINE_MUTEX(mut9); + +static DECLARE_RWSEM(rwsem0); +static DECLARE_RWSEM(rwsem1); +static DECLARE_RWSEM(rwsem2); +static DECLARE_RWSEM(rwsem3); +static DECLARE_RWSEM(rwsem4); +static DECLARE_RWSEM(rwsem5); +static DECLARE_RWSEM(rwsem6); +static DECLARE_RWSEM(rwsem7); +static DECLARE_RWSEM(rwsem8); +static DECLARE_RWSEM(rwsem9); + +DEFINE_STATIC_SRCU(srcu0); +DEFINE_STATIC_SRCU(srcu1); +DEFINE_STATIC_SRCU(srcu2); +DEFINE_STATIC_SRCU(srcu3); +DEFINE_STATIC_SRCU(srcu4); +DEFINE_STATIC_SRCU(srcu5); +DEFINE_STATIC_SRCU(srcu6); +DEFINE_STATIC_SRCU(srcu7); +DEFINE_STATIC_SRCU(srcu8); +DEFINE_STATIC_SRCU(srcu9); + +static int srcu_lockdep_next(const char *f, const char *fl, const char *fs, const char *fu, int i, + int cyclelen, int deadlock) +{ + int j = i + 1; + + if (j >= cyclelen) + j = deadlock ? 0 : -1; + if (j >= 0) + pr_info("%s: %s(%d), %s(%d), %s(%d)\n", f, fl, i, fs, j, fu, i); + else + pr_info("%s: %s(%d), %s(%d)\n", f, fl, i, fu, i); + return j; +} + +// Test lockdep on SRCU-based deadlock scenarios. +static void rcu_torture_init_srcu_lockdep(void) +{ + int cyclelen; + int deadlock; + bool err = false; + int i; + int j; + int idx; + struct mutex *muts[] = { &mut0, &mut1, &mut2, &mut3, &mut4, + &mut5, &mut6, &mut7, &mut8, &mut9 }; + struct rw_semaphore *rwsems[] = { &rwsem0, &rwsem1, &rwsem2, &rwsem3, &rwsem4, + &rwsem5, &rwsem6, &rwsem7, &rwsem8, &rwsem9 }; + struct srcu_struct *srcus[] = { &srcu0, &srcu1, &srcu2, &srcu3, &srcu4, + &srcu5, &srcu6, &srcu7, &srcu8, &srcu9 }; + int testtype; + + if (!test_srcu_lockdep) + return; + + deadlock = test_srcu_lockdep / 1000; + testtype = (test_srcu_lockdep / 10) % 100; + cyclelen = test_srcu_lockdep % 10; + WARN_ON_ONCE(ARRAY_SIZE(muts) != ARRAY_SIZE(srcus)); + if (WARN_ONCE(deadlock != !!deadlock, + "%s: test_srcu_lockdep=%d and deadlock digit %d must be zero or one.\n", + __func__, test_srcu_lockdep, deadlock)) + err = true; + if (WARN_ONCE(cyclelen <= 0, + "%s: test_srcu_lockdep=%d and cycle-length digit %d must be greater than zero.\n", + __func__, test_srcu_lockdep, cyclelen)) + err = true; + if (err) + goto err_out; + + if (testtype == 0) { + pr_info("%s: test_srcu_lockdep = %05d: SRCU %d-way %sdeadlock.\n", + __func__, test_srcu_lockdep, cyclelen, deadlock ? "" : "non-"); + if (deadlock && cyclelen == 1) + pr_info("%s: Expect hang.\n", __func__); + for (i = 0; i < cyclelen; i++) { + j = srcu_lockdep_next(__func__, "srcu_read_lock", "synchronize_srcu", + "srcu_read_unlock", i, cyclelen, deadlock); + idx = srcu_read_lock(srcus[i]); + if (j >= 0) + synchronize_srcu(srcus[j]); + srcu_read_unlock(srcus[i], idx); + } + return; + } + + if (testtype == 1) { + pr_info("%s: test_srcu_lockdep = %05d: SRCU/mutex %d-way %sdeadlock.\n", + __func__, test_srcu_lockdep, cyclelen, deadlock ? "" : "non-"); + for (i = 0; i < cyclelen; i++) { + pr_info("%s: srcu_read_lock(%d), mutex_lock(%d), mutex_unlock(%d), srcu_read_unlock(%d)\n", + __func__, i, i, i, i); + idx = srcu_read_lock(srcus[i]); + mutex_lock(muts[i]); + mutex_unlock(muts[i]); + srcu_read_unlock(srcus[i], idx); + + j = srcu_lockdep_next(__func__, "mutex_lock", "synchronize_srcu", + "mutex_unlock", i, cyclelen, deadlock); + mutex_lock(muts[i]); + if (j >= 0) + synchronize_srcu(srcus[j]); + mutex_unlock(muts[i]); + } + return; + } + + if (testtype == 2) { + pr_info("%s: test_srcu_lockdep = %05d: SRCU/rwsem %d-way %sdeadlock.\n", + __func__, test_srcu_lockdep, cyclelen, deadlock ? "" : "non-"); + for (i = 0; i < cyclelen; i++) { + pr_info("%s: srcu_read_lock(%d), down_read(%d), up_read(%d), srcu_read_unlock(%d)\n", + __func__, i, i, i, i); + idx = srcu_read_lock(srcus[i]); + down_read(rwsems[i]); + up_read(rwsems[i]); + srcu_read_unlock(srcus[i], idx); + + j = srcu_lockdep_next(__func__, "down_write", "synchronize_srcu", + "up_write", i, cyclelen, deadlock); + down_write(rwsems[i]); + if (j >= 0) + synchronize_srcu(srcus[j]); + up_write(rwsems[i]); + } + return; + } + +err_out: + pr_info("%s: test_srcu_lockdep = %05d does nothing.\n", __func__, test_srcu_lockdep); + pr_info("%s: test_srcu_lockdep = DNNL.\n", __func__); + pr_info("%s: D: Deadlock if nonzero.\n", __func__); + pr_info("%s: NN: Test number, 0=SRCU, 1=SRCU/mutex, 2=SRCU/rwsem.\n", __func__); + pr_info("%s: L: Cycle length.\n", __func__); +} + static int __init rcu_torture_init(void) { @@ -3504,6 +3653,8 @@ rcu_torture_init(void) if (cur_ops->init) cur_ops->init();
+ rcu_torture_init_srcu_lockdep(); + if (nreaders >= 0) { nrealreaders = nreaders; } else {
From: "Paul E. McKenney" paulmck@kernel.org
Add a test number 3 that creates deadlock cycles involving one RCU Tasks Trace step and L-1 SRCU steps. Please note that lockdep will not detect these deadlocks until synchronize_rcu_tasks_trace() is marked with lockdep's new "sync" annotation, which will probably not happen until some time after these markings prove their worth on SRCU.
Please note that these tests are available only in kernels built with CONFIG_TASKS_TRACE_RCU=y.
Signed-off-by: Paul E. McKenney paulmck@kernel.org Signed-off-by: Boqun Feng boqun.feng@gmail.com --- kernel/rcu/rcutorture.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c index 80ff9a743d31..9efb8258a272 100644 --- a/kernel/rcu/rcutorture.c +++ b/kernel/rcu/rcutorture.c @@ -3604,12 +3604,46 @@ static void rcu_torture_init_srcu_lockdep(void) return; }
+#ifdef CONFIG_TASKS_TRACE_RCU + if (testtype == 3) { + pr_info("%s: test_srcu_lockdep = %05d: SRCU and Tasks Trace RCU %d-way %sdeadlock.\n", + __func__, test_srcu_lockdep, cyclelen, deadlock ? "" : "non-"); + if (deadlock && cyclelen == 1) + pr_info("%s: Expect hang.\n", __func__); + for (i = 0; i < cyclelen; i++) { + char *fl = i == 0 ? "rcu_read_lock_trace" : "srcu_read_lock"; + char *fs = i == cyclelen - 1 ? "synchronize_rcu_tasks_trace" + : "synchronize_srcu"; + char *fu = i == 0 ? "rcu_read_unlock_trace" : "srcu_read_unlock"; + + j = srcu_lockdep_next(__func__, fl, fs, fu, i, cyclelen, deadlock); + if (i == 0) + rcu_read_lock_trace(); + else + idx = srcu_read_lock(srcus[i]); + if (j >= 0) { + if (i == cyclelen - 1) + synchronize_rcu_tasks_trace(); + else + synchronize_srcu(srcus[j]); + } + if (i == 0) + rcu_read_unlock_trace(); + else + srcu_read_unlock(srcus[i], idx); + } + return; + } +#endif // #ifdef CONFIG_TASKS_TRACE_RCU + err_out: pr_info("%s: test_srcu_lockdep = %05d does nothing.\n", __func__, test_srcu_lockdep); pr_info("%s: test_srcu_lockdep = DNNL.\n", __func__); pr_info("%s: D: Deadlock if nonzero.\n", __func__); - pr_info("%s: NN: Test number, 0=SRCU, 1=SRCU/mutex, 2=SRCU/rwsem.\n", __func__); + pr_info("%s: NN: Test number, 0=SRCU, 1=SRCU/mutex, 2=SRCU/rwsem, 3=SRCU/Tasks Trace RCU.\n", __func__); pr_info("%s: L: Cycle length.\n", __func__); + if (!IS_ENABLED(CONFIG_TASKS_TRACE_RCU)) + pr_info("%s: NN=3 disallowed because kernel is built with CONFIG_TASKS_TRACE_RCU=n\n", __func__); }
static int __init
From: "Paul E. McKenney" paulmck@kernel.org
This commit adds an srcu_lockdep.sh script that checks whether lockdep correctly classifies SRCU-based, SRCU/mutex-based, and SRCU/rwsem-based deadlocks.
Signed-off-by: Paul E. McKenney paulmck@kernel.org [ boqun: Fix "RCUTORTURE" with "$RCUTORTURE" ] Signed-off-by: Boqun Feng boqun.feng@gmail.com --- .../selftests/rcutorture/bin/srcu_lockdep.sh | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100755 tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
diff --git a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh new file mode 100755 index 000000000000..961932754684 --- /dev/null +++ b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh @@ -0,0 +1,73 @@ +#!/bin/bash +# +# Run SRCU-lockdep tests and report any that fail to meet expectations. + +usage () { + echo "Usage: $scriptname optional arguments:" + echo " --datestamp string" + exit 1 +} + +ds=`date +%Y.%m.%d-%H.%M.%S`-srcu_lockdep +scriptname="$0" + +T="`mktemp -d ${TMPDIR-/tmp}/srcu_lockdep.sh.XXXXXX`" +trap 'rm -rf $T' 0 + +RCUTORTURE="`pwd`/tools/testing/selftests/rcutorture"; export RCUTORTURE +PATH=${RCUTORTURE}/bin:$PATH; export PATH +. functions.sh + +while test $# -gt 0 +do + case "$1" in + --datestamp) + checkarg --datestamp "(relative pathname)" "$#" "$2" '^[a-zA-Z0-9._/-]*$' '^--' + ds=$2 + shift + ;; + *) + echo Unknown argument $1 + usage + ;; + esac + shift +done + +err= +nerrs=0 +for d in 0 1 +do + for t in 0 1 2 + do + for c in 1 2 3 + do + err= + val=$((d*1000+t*10+c)) + tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 5s --configs "SRCU-P" --bootargs "rcutorture.test_srcu_lockdep=$val" --trust-make --datestamp "$ds/$val" > "$T/kvm.sh.out" 2>&1 + ret=$? + mv "$T/kvm.sh.out" "$RCUTORTURE/res/$ds/$val" + if test "$d" -ne 0 && test "$ret" -eq 0 + then + err=1 + echo -n Unexpected success for > "$RCUTORTURE/res/$ds/$val/kvm.sh.err" + fi + if test "$d" -eq 0 && test "$ret" -ne 0 + then + err=1 + echo -n Unexpected failure for > "$RCUTORTURE/res/$ds/$val/kvm.sh.err" + fi + if test -n "$err" + then + grep "rcu_torture_init_srcu_lockdep: test_srcu_lockdep = " "$RCUTORTURE/res/$ds/$val/SRCU-P/console.log" | sed -e 's/^.*rcu_torture_init_srcu_lockdep://' >> "$RCUTORTURE/res/$ds/$val/kvm.sh.err" + cat "$RCUTORTURE/res/$ds/$val/kvm.sh.err" + nerrs=$((nerrs+1)) + fi + done + done +done +if test "$nerrs" -ne 0 +then + exit 1 +fi +exit 0
Hi Paul,
On Thu, Mar 16, 2023 at 08:13:39PM -0700, Boqun Feng wrote:
From: "Paul E. McKenney" paulmck@kernel.org
This commit adds an srcu_lockdep.sh script that checks whether lockdep correctly classifies SRCU-based, SRCU/mutex-based, and SRCU/rwsem-based deadlocks.
Signed-off-by: Paul E. McKenney paulmck@kernel.org [ boqun: Fix "RCUTORTURE" with "$RCUTORTURE" ] Signed-off-by: Boqun Feng boqun.feng@gmail.com
.../selftests/rcutorture/bin/srcu_lockdep.sh | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100755 tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
diff --git a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh new file mode 100755 index 000000000000..961932754684 --- /dev/null +++ b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
Could you provide the SPDX header and copyright bits for this newly added file? For small changes I can do it myself, however this is about licenses and copyright, so I need it from you, thanks!
Regards, Boqun
@@ -0,0 +1,73 @@ +#!/bin/bash +# +# Run SRCU-lockdep tests and report any that fail to meet expectations.
+usage () {
- echo "Usage: $scriptname optional arguments:"
- echo " --datestamp string"
- exit 1
+}
+ds=`date +%Y.%m.%d-%H.%M.%S`-srcu_lockdep +scriptname="$0"
+T="`mktemp -d ${TMPDIR-/tmp}/srcu_lockdep.sh.XXXXXX`" +trap 'rm -rf $T' 0
+RCUTORTURE="`pwd`/tools/testing/selftests/rcutorture"; export RCUTORTURE +PATH=${RCUTORTURE}/bin:$PATH; export PATH +. functions.sh
+while test $# -gt 0 +do
- case "$1" in
- --datestamp)
checkarg --datestamp "(relative pathname)" "$#" "$2" '^[a-zA-Z0-9._/-]*$' '^--'ds=$2shift;;- *)
echo Unknown argument $1usage;;- esac
- shift
+done
+err= +nerrs=0 +for d in 0 1 +do
- for t in 0 1 2
- do
for c in 1 2 3doerr=val=$((d*1000+t*10+c))tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 5s --configs "SRCU-P" --bootargs "rcutorture.test_srcu_lockdep=$val" --trust-make --datestamp "$ds/$val" > "$T/kvm.sh.out" 2>&1ret=$?mv "$T/kvm.sh.out" "$RCUTORTURE/res/$ds/$val"if test "$d" -ne 0 && test "$ret" -eq 0thenerr=1echo -n Unexpected success for > "$RCUTORTURE/res/$ds/$val/kvm.sh.err"fiif test "$d" -eq 0 && test "$ret" -ne 0thenerr=1echo -n Unexpected failure for > "$RCUTORTURE/res/$ds/$val/kvm.sh.err"fiif test -n "$err"thengrep "rcu_torture_init_srcu_lockdep: test_srcu_lockdep = " "$RCUTORTURE/res/$ds/$val/SRCU-P/console.log" | sed -e 's/^.*rcu_torture_init_srcu_lockdep://' >> "$RCUTORTURE/res/$ds/$val/kvm.sh.err"cat "$RCUTORTURE/res/$ds/$val/kvm.sh.err"nerrs=$((nerrs+1))fidone- done
+done +if test "$nerrs" -ne 0 +then
- exit 1
+fi
+exit 0
2.39.2
On Mon, Mar 20, 2023 at 11:19:05AM -0700, Boqun Feng wrote:
Hi Paul,
On Thu, Mar 16, 2023 at 08:13:39PM -0700, Boqun Feng wrote:
From: "Paul E. McKenney" paulmck@kernel.org
This commit adds an srcu_lockdep.sh script that checks whether lockdep correctly classifies SRCU-based, SRCU/mutex-based, and SRCU/rwsem-based deadlocks.
Signed-off-by: Paul E. McKenney paulmck@kernel.org [ boqun: Fix "RCUTORTURE" with "$RCUTORTURE" ] Signed-off-by: Boqun Feng boqun.feng@gmail.com
.../selftests/rcutorture/bin/srcu_lockdep.sh | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100755 tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
diff --git a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh new file mode 100755 index 000000000000..961932754684 --- /dev/null +++ b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
Could you provide the SPDX header and copyright bits for this newly added file? For small changes I can do it myself, however this is about licenses and copyright, so I need it from you, thanks!
Good catch, thank you!
Would you like a delta patch to merge into your existing one, or would you prefer a replacement patch? Either way works for me.
Thanx, Paul
Regards, Boqun
@@ -0,0 +1,73 @@ +#!/bin/bash +# +# Run SRCU-lockdep tests and report any that fail to meet expectations.
+usage () {
- echo "Usage: $scriptname optional arguments:"
- echo " --datestamp string"
- exit 1
+}
+ds=`date +%Y.%m.%d-%H.%M.%S`-srcu_lockdep +scriptname="$0"
+T="`mktemp -d ${TMPDIR-/tmp}/srcu_lockdep.sh.XXXXXX`" +trap 'rm -rf $T' 0
+RCUTORTURE="`pwd`/tools/testing/selftests/rcutorture"; export RCUTORTURE +PATH=${RCUTORTURE}/bin:$PATH; export PATH +. functions.sh
+while test $# -gt 0 +do
- case "$1" in
- --datestamp)
checkarg --datestamp "(relative pathname)" "$#" "$2" '^[a-zA-Z0-9._/-]*$' '^--'ds=$2shift;;- *)
echo Unknown argument $1usage;;- esac
- shift
+done
+err= +nerrs=0 +for d in 0 1 +do
- for t in 0 1 2
- do
for c in 1 2 3doerr=val=$((d*1000+t*10+c))tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 5s --configs "SRCU-P" --bootargs "rcutorture.test_srcu_lockdep=$val" --trust-make --datestamp "$ds/$val" > "$T/kvm.sh.out" 2>&1ret=$?mv "$T/kvm.sh.out" "$RCUTORTURE/res/$ds/$val"if test "$d" -ne 0 && test "$ret" -eq 0thenerr=1echo -n Unexpected success for > "$RCUTORTURE/res/$ds/$val/kvm.sh.err"fiif test "$d" -eq 0 && test "$ret" -ne 0thenerr=1echo -n Unexpected failure for > "$RCUTORTURE/res/$ds/$val/kvm.sh.err"fiif test -n "$err"thengrep "rcu_torture_init_srcu_lockdep: test_srcu_lockdep = " "$RCUTORTURE/res/$ds/$val/SRCU-P/console.log" | sed -e 's/^.*rcu_torture_init_srcu_lockdep://' >> "$RCUTORTURE/res/$ds/$val/kvm.sh.err"cat "$RCUTORTURE/res/$ds/$val/kvm.sh.err"nerrs=$((nerrs+1))fidone- done
+done +if test "$nerrs" -ne 0 +then
- exit 1
+fi
+exit 0
2.39.2
On Mon, Mar 20, 2023 at 12:09:00PM -0700, Paul E. McKenney wrote:
On Mon, Mar 20, 2023 at 11:19:05AM -0700, Boqun Feng wrote:
Hi Paul,
On Thu, Mar 16, 2023 at 08:13:39PM -0700, Boqun Feng wrote:
From: "Paul E. McKenney" paulmck@kernel.org
This commit adds an srcu_lockdep.sh script that checks whether lockdep correctly classifies SRCU-based, SRCU/mutex-based, and SRCU/rwsem-based deadlocks.
Signed-off-by: Paul E. McKenney paulmck@kernel.org [ boqun: Fix "RCUTORTURE" with "$RCUTORTURE" ] Signed-off-by: Boqun Feng boqun.feng@gmail.com
.../selftests/rcutorture/bin/srcu_lockdep.sh | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100755 tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
diff --git a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh new file mode 100755 index 000000000000..961932754684 --- /dev/null +++ b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
Could you provide the SPDX header and copyright bits for this newly added file? For small changes I can do it myself, however this is about licenses and copyright, so I need it from you, thanks!
Good catch, thank you!
Would you like a delta patch to merge into your existing one, or would you prefer a replacement patch? Either way works for me.
A delta patch if that's not much trouble. I will fold it into this one.
Regards, Boqun
Thanx, PaulRegards, Boqun
@@ -0,0 +1,73 @@ +#!/bin/bash +# +# Run SRCU-lockdep tests and report any that fail to meet expectations.
+usage () {
- echo "Usage: $scriptname optional arguments:"
- echo " --datestamp string"
- exit 1
+}
+ds=`date +%Y.%m.%d-%H.%M.%S`-srcu_lockdep +scriptname="$0"
+T="`mktemp -d ${TMPDIR-/tmp}/srcu_lockdep.sh.XXXXXX`" +trap 'rm -rf $T' 0
+RCUTORTURE="`pwd`/tools/testing/selftests/rcutorture"; export RCUTORTURE +PATH=${RCUTORTURE}/bin:$PATH; export PATH +. functions.sh
+while test $# -gt 0 +do
- case "$1" in
- --datestamp)
checkarg --datestamp "(relative pathname)" "$#" "$2" '^[a-zA-Z0-9._/-]*$' '^--'ds=$2shift;;- *)
echo Unknown argument $1usage;;- esac
- shift
+done
+err= +nerrs=0 +for d in 0 1 +do
- for t in 0 1 2
- do
for c in 1 2 3doerr=val=$((d*1000+t*10+c))tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 5s --configs "SRCU-P" --bootargs "rcutorture.test_srcu_lockdep=$val" --trust-make --datestamp "$ds/$val" > "$T/kvm.sh.out" 2>&1ret=$?mv "$T/kvm.sh.out" "$RCUTORTURE/res/$ds/$val"if test "$d" -ne 0 && test "$ret" -eq 0thenerr=1echo -n Unexpected success for > "$RCUTORTURE/res/$ds/$val/kvm.sh.err"fiif test "$d" -eq 0 && test "$ret" -ne 0thenerr=1echo -n Unexpected failure for > "$RCUTORTURE/res/$ds/$val/kvm.sh.err"fiif test -n "$err"thengrep "rcu_torture_init_srcu_lockdep: test_srcu_lockdep = " "$RCUTORTURE/res/$ds/$val/SRCU-P/console.log" | sed -e 's/^.*rcu_torture_init_srcu_lockdep://' >> "$RCUTORTURE/res/$ds/$val/kvm.sh.err"cat "$RCUTORTURE/res/$ds/$val/kvm.sh.err"nerrs=$((nerrs+1))fidone- done
+done +if test "$nerrs" -ne 0 +then
- exit 1
+fi
+exit 0
2.39.2
On Mon, Mar 20, 2023 at 12:28:05PM -0700, Boqun Feng wrote:
On Mon, Mar 20, 2023 at 12:09:00PM -0700, Paul E. McKenney wrote:
On Mon, Mar 20, 2023 at 11:19:05AM -0700, Boqun Feng wrote:
Hi Paul,
On Thu, Mar 16, 2023 at 08:13:39PM -0700, Boqun Feng wrote:
From: "Paul E. McKenney" paulmck@kernel.org
This commit adds an srcu_lockdep.sh script that checks whether lockdep correctly classifies SRCU-based, SRCU/mutex-based, and SRCU/rwsem-based deadlocks.
Signed-off-by: Paul E. McKenney paulmck@kernel.org [ boqun: Fix "RCUTORTURE" with "$RCUTORTURE" ] Signed-off-by: Boqun Feng boqun.feng@gmail.com
.../selftests/rcutorture/bin/srcu_lockdep.sh | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100755 tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
diff --git a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh new file mode 100755 index 000000000000..961932754684 --- /dev/null +++ b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
Could you provide the SPDX header and copyright bits for this newly added file? For small changes I can do it myself, however this is about licenses and copyright, so I need it from you, thanks!
Good catch, thank you!
Would you like a delta patch to merge into your existing one, or would you prefer a replacement patch? Either way works for me.
A delta patch if that's not much trouble. I will fold it into this one.
Here you go!
Thanx, Paul
------------------------------------------------------------------------
rcutorture: Add proper comment header to srcu_lockdep.sh
This patch adds a proper comment header to srcu_lockdep.sh, and is intended to be folded into 9dc68f40c665 ("rcutorture: Add srcu_lockdep.sh").
Signed-off-by: Paul E. McKenney paulmck@kernel.org
diff --git a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh index 961932754684..2e63ef009d59 100755 --- a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh +++ b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh @@ -1,6 +1,11 @@ #!/bin/bash +# SPDX-License-Identifier: GPL-2.0+ # # Run SRCU-lockdep tests and report any that fail to meet expectations. +# +# Copyright (C) 2021 Meta Platforms, Inc. +# +# Authors: Paul E. McKenney paulmck@kernel.org
usage () { echo "Usage: $scriptname optional arguments:"
On Mon, Mar 20, 2023 at 01:22:24PM -0700, Paul E. McKenney wrote:
On Mon, Mar 20, 2023 at 12:28:05PM -0700, Boqun Feng wrote:
On Mon, Mar 20, 2023 at 12:09:00PM -0700, Paul E. McKenney wrote:
On Mon, Mar 20, 2023 at 11:19:05AM -0700, Boqun Feng wrote:
Hi Paul,
On Thu, Mar 16, 2023 at 08:13:39PM -0700, Boqun Feng wrote:
From: "Paul E. McKenney" paulmck@kernel.org
This commit adds an srcu_lockdep.sh script that checks whether lockdep correctly classifies SRCU-based, SRCU/mutex-based, and SRCU/rwsem-based deadlocks.
Signed-off-by: Paul E. McKenney paulmck@kernel.org [ boqun: Fix "RCUTORTURE" with "$RCUTORTURE" ] Signed-off-by: Boqun Feng boqun.feng@gmail.com
.../selftests/rcutorture/bin/srcu_lockdep.sh | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100755 tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
diff --git a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh new file mode 100755 index 000000000000..961932754684 --- /dev/null +++ b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
Could you provide the SPDX header and copyright bits for this newly added file? For small changes I can do it myself, however this is about licenses and copyright, so I need it from you, thanks!
Good catch, thank you!
Would you like a delta patch to merge into your existing one, or would you prefer a replacement patch? Either way works for me.
A delta patch if that's not much trouble. I will fold it into this one.
Here you go!
Thanks! Fold it in patch #7.
Regards, Boqun
Thanx, Paul
rcutorture: Add proper comment header to srcu_lockdep.sh
This patch adds a proper comment header to srcu_lockdep.sh, and is intended to be folded into 9dc68f40c665 ("rcutorture: Add srcu_lockdep.sh").
Signed-off-by: Paul E. McKenney paulmck@kernel.org
diff --git a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh index 961932754684..2e63ef009d59 100755 --- a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh +++ b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh @@ -1,6 +1,11 @@ #!/bin/bash +# SPDX-License-Identifier: GPL-2.0+ # # Run SRCU-lockdep tests and report any that fail to meet expectations. +# +# Copyright (C) 2021 Meta Platforms, Inc. +# +# Authors: Paul E. McKenney paulmck@kernel.org usage () { echo "Usage: $scriptname optional arguments:"
On Mon, Mar 20, 2023 at 01:26:39PM -0700, Boqun Feng wrote:
On Mon, Mar 20, 2023 at 01:22:24PM -0700, Paul E. McKenney wrote:
On Mon, Mar 20, 2023 at 12:28:05PM -0700, Boqun Feng wrote:
On Mon, Mar 20, 2023 at 12:09:00PM -0700, Paul E. McKenney wrote:
On Mon, Mar 20, 2023 at 11:19:05AM -0700, Boqun Feng wrote:
Hi Paul,
On Thu, Mar 16, 2023 at 08:13:39PM -0700, Boqun Feng wrote:
From: "Paul E. McKenney" paulmck@kernel.org
This commit adds an srcu_lockdep.sh script that checks whether lockdep correctly classifies SRCU-based, SRCU/mutex-based, and SRCU/rwsem-based deadlocks.
Signed-off-by: Paul E. McKenney paulmck@kernel.org [ boqun: Fix "RCUTORTURE" with "$RCUTORTURE" ] Signed-off-by: Boqun Feng boqun.feng@gmail.com
.../selftests/rcutorture/bin/srcu_lockdep.sh | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100755 tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
diff --git a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh new file mode 100755 index 000000000000..961932754684 --- /dev/null +++ b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh
Could you provide the SPDX header and copyright bits for this newly added file? For small changes I can do it myself, however this is about licenses and copyright, so I need it from you, thanks!
Good catch, thank you!
Would you like a delta patch to merge into your existing one, or would you prefer a replacement patch? Either way works for me.
A delta patch if that's not much trouble. I will fold it into this one.
Here you go!
Thanks! Fold it in patch #7.
Thank you!
And this might be a problem in my current process when handing off to others. I normally don't run checkpatch until just before a send out the patches, which means that none of the commits I handed off to you guys had been checkpatched. There was I time I tried a checkpatch hook in git, but the false-positive rate made that a non-starter.
Thanx, Paul
Regards, Boqun
Thanx, Paul
rcutorture: Add proper comment header to srcu_lockdep.sh
This patch adds a proper comment header to srcu_lockdep.sh, and is intended to be folded into 9dc68f40c665 ("rcutorture: Add srcu_lockdep.sh").
Signed-off-by: Paul E. McKenney paulmck@kernel.org
diff --git a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh index 961932754684..2e63ef009d59 100755 --- a/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh +++ b/tools/testing/selftests/rcutorture/bin/srcu_lockdep.sh @@ -1,6 +1,11 @@ #!/bin/bash +# SPDX-License-Identifier: GPL-2.0+ # # Run SRCU-lockdep tests and report any that fail to meet expectations. +# +# Copyright (C) 2021 Meta Platforms, Inc. +# +# Authors: Paul E. McKenney paulmck@kernel.org usage () { echo "Usage: $scriptname optional arguments:"
linux-kselftest-mirror@lists.linaro.org
-
Boqun Feng -
Davidlohr Bueso -
Paul E. McKenney -
Peter Zijlstra