In order ot extend locktorture to support lock nesting, add
nested_lock() and nested_unlock() hooks to the torture ops.
These take a 32bit lockset mask which is generated at random,
so some number of locks will be taken before the main lock is
taken and released afterwards.
Additionally, add nlocks module parameter to allow specifying
the number of nested locks to be used.
This has been helpful to uncover issues in the proxy-exec
series development.
This was inspired by locktorture extensions originally implemented
by Connor O'Brien, for stress testing the proxy-execution series:
https://lore.kernel.org/lkml/[email protected]/
Comments or feedback would be greatly appreciated!
Cc: Davidlohr Bueso <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Josh Triplett <[email protected]>
Cc: Joel Fernandes <[email protected]>
Cc: Juri Lelli <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Dietmar Eggemann <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
kernel/locking/locktorture.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index 9c2fb613a55d..f4fbd3194654 100644
--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -48,6 +48,9 @@ torture_param(int, stat_interval, 60,
torture_param(int, stutter, 5, "Number of jiffies to run/halt test, 0=disable");
torture_param(int, verbose, 1,
"Enable verbose debugging printk()s");
+torture_param(int, nlocks, 0, "Number of nested locks");
+/* Going much higher trips "BUG: MAX_LOCKDEP_CHAIN_HLOCKS too low!" errors */
+#define MAX_LOCKS 8
static char *torture_type = "spin_lock";
module_param(torture_type, charp, 0444);
@@ -76,10 +79,12 @@ static void lock_torture_cleanup(void);
struct lock_torture_ops {
void (*init)(void);
void (*exit)(void);
+ int (*nested_lock)(int tid, u32 lockset);
int (*writelock)(int tid);
void (*write_delay)(struct torture_random_state *trsp);
void (*task_boost)(struct torture_random_state *trsp);
void (*writeunlock)(int tid);
+ void (*nested_unlock)(int tid, u32 lockset);
int (*readlock)(int tid);
void (*read_delay)(struct torture_random_state *trsp);
void (*readunlock)(int tid);
@@ -669,6 +674,7 @@ static int lock_torture_writer(void *arg)
struct lock_stress_stats *lwsp = arg;
int tid = lwsp - cxt.lwsa;
DEFINE_TORTURE_RANDOM(rand);
+ u32 lockset_mask;
VERBOSE_TOROUT_STRING("lock_torture_writer task started");
set_user_nice(current, MAX_NICE);
@@ -677,7 +683,10 @@ static int lock_torture_writer(void *arg)
if ((torture_random(&rand) & 0xfffff) == 0)
schedule_timeout_uninterruptible(1);
+ lockset_mask = torture_random(&rand);
cxt.cur_ops->task_boost(&rand);
+ if (cxt.cur_ops->nested_lock)
+ cxt.cur_ops->nested_lock(tid, lockset_mask);
cxt.cur_ops->writelock(tid);
if (WARN_ON_ONCE(lock_is_write_held))
lwsp->n_lock_fail++;
@@ -690,6 +699,8 @@ static int lock_torture_writer(void *arg)
lock_is_write_held = false;
WRITE_ONCE(last_lock_release, jiffies);
cxt.cur_ops->writeunlock(tid);
+ if (cxt.cur_ops->nested_unlock)
+ cxt.cur_ops->nested_unlock(tid, lockset_mask);
stutter_wait("lock_torture_writer");
} while (!torture_must_stop());
@@ -830,11 +841,11 @@ lock_torture_print_module_parms(struct lock_torture_ops *cur_ops,
const char *tag)
{
pr_alert("%s" TORTURE_FLAG
- "--- %s%s: nwriters_stress=%d nreaders_stress=%d stat_interval=%d verbose=%d shuffle_interval=%d stutter=%d shutdown_secs=%d onoff_interval=%d onoff_holdoff=%d\n",
+ "--- %s%s: nwriters_stress=%d nreaders_stress=%d nlocks=%d stat_interval=%d verbose=%d shuffle_interval=%d stutter=%d shutdown_secs=%d onoff_interval=%d onoff_holdoff=%d\n",
torture_type, tag, cxt.debug_lock ? " [debug]": "",
- cxt.nrealwriters_stress, cxt.nrealreaders_stress, stat_interval,
- verbose, shuffle_interval, stutter, shutdown_secs,
- onoff_interval, onoff_holdoff);
+ cxt.nrealwriters_stress, cxt.nrealreaders_stress, nlocks,
+ stat_interval, verbose, shuffle_interval, stutter,
+ shutdown_secs, onoff_interval, onoff_holdoff);
}
static void lock_torture_cleanup(void)
@@ -1053,6 +1064,10 @@ static int __init lock_torture_init(void)
}
}
+ /* cap nlocks to MAX_LOCKS */
+ if (nlocks > MAX_LOCKS)
+ nlocks = MAX_LOCKS;
+
if (cxt.cur_ops->readlock) {
reader_tasks = kcalloc(cxt.nrealreaders_stress,
sizeof(reader_tasks[0]),
--
2.39.1.519.gcb327c4b5f-goog
This patch adds randomized nested locking to the mutex torture
tests.
This was inspired by locktorture extensions originally implemented
by Connor O'Brien, for stress testing the proxy-execution series:
https://lore.kernel.org/lkml/[email protected]/
Comments or feedback would be greatly appreciated!
Cc: Davidlohr Bueso <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Josh Triplett <[email protected]>
Cc: Joel Fernandes <[email protected]>
Cc: Juri Lelli <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Dietmar Eggemann <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
kernel/locking/locktorture.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index 27d92ce36836..5fb17a5057b5 100644
--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -506,6 +506,28 @@ static struct lock_torture_ops ww_mutex_lock_ops = {
#ifdef CONFIG_RT_MUTEXES
static DEFINE_RT_MUTEX(torture_rtmutex);
+static struct rt_mutex torture_nested_rtmutexes[MAX_LOCKS];
+static struct lock_class_key nested_rtmutex_keys[MAX_LOCKS];
+
+static void torture_rtmutex_init(void)
+{
+ int i;
+
+ for (i = 0; i < MAX_LOCKS; i++)
+ __rt_mutex_init(&torture_nested_rtmutexes[i], __func__,
+ &nested_rtmutex_keys[i]);
+}
+
+static int torture_rtmutex_nested_lock(int tid __maybe_unused,
+ u32 lockset)
+{
+ int i;
+
+ for (i = 0; i < nlocks; i++)
+ if (lockset & (1 << i))
+ rt_mutex_lock(&torture_nested_rtmutexes[i]);
+ return 0;
+}
static int torture_rtmutex_lock(int tid __maybe_unused)
__acquires(torture_rtmutex)
@@ -570,11 +592,24 @@ __releases(torture_rtmutex)
rt_mutex_unlock(&torture_rtmutex);
}
+static void torture_rtmutex_nested_unlock(int tid __maybe_unused,
+ u32 lockset)
+{
+ int i;
+
+ for (i = nlocks - 1; i >= 0; i--)
+ if (lockset & (1 << i))
+ rt_mutex_unlock(&torture_nested_rtmutexes[i]);
+}
+
static struct lock_torture_ops rtmutex_lock_ops = {
+ .init = torture_rtmutex_init,
+ .nested_lock = torture_rtmutex_nested_lock,
.writelock = torture_rtmutex_lock,
.write_delay = torture_rtmutex_delay,
.task_boost = torture_rtmutex_boost,
.writeunlock = torture_rtmutex_unlock,
+ .nested_unlock = torture_rtmutex_nested_unlock,
.readlock = NULL,
.read_delay = NULL,
.readunlock = NULL,
--
2.39.1.519.gcb327c4b5f-goog
If we're using nested locking to stress things, occasionally
skip taking the main lock, so that we can get some different
contention patterns between the writers (to hopefully get two
disjoint blocked trees)
This patch was inspired by earlier work by Connor O'Brien.
Comments or feedback would be greatly appreciated!
Cc: Davidlohr Bueso <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Josh Triplett <[email protected]>
Cc: Joel Fernandes <[email protected]>
Cc: Juri Lelli <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Dietmar Eggemann <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
kernel/locking/locktorture.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index 5fb17a5057b5..6f56dcb8a496 100644
--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -745,6 +745,7 @@ static int lock_torture_writer(void *arg)
int tid = lwsp - cxt.lwsa;
DEFINE_TORTURE_RANDOM(rand);
u32 lockset_mask;
+ bool skip_main_lock;
VERBOSE_TOROUT_STRING("lock_torture_writer task started");
set_user_nice(current, MAX_NICE);
@@ -754,21 +755,28 @@ static int lock_torture_writer(void *arg)
schedule_timeout_uninterruptible(1);
lockset_mask = torture_random(&rand);
+ skip_main_lock = nlocks && !(torture_random(&rand) % 100);
+
cxt.cur_ops->task_boost(&rand);
if (cxt.cur_ops->nested_lock)
cxt.cur_ops->nested_lock(tid, lockset_mask);
- cxt.cur_ops->writelock(tid);
- if (WARN_ON_ONCE(lock_is_write_held))
- lwsp->n_lock_fail++;
- lock_is_write_held = true;
- if (WARN_ON_ONCE(atomic_read(&lock_is_read_held)))
- lwsp->n_lock_fail++; /* rare, but... */
- lwsp->n_lock_acquired++;
+ if (!skip_main_lock) {
+ cxt.cur_ops->writelock(tid);
+ if (WARN_ON_ONCE(lock_is_write_held))
+ lwsp->n_lock_fail++;
+ lock_is_write_held = true;
+ if (WARN_ON_ONCE(atomic_read(&lock_is_read_held)))
+ lwsp->n_lock_fail++; /* rare, but... */
+
+ lwsp->n_lock_acquired++;
+ }
cxt.cur_ops->write_delay(&rand);
- lock_is_write_held = false;
- WRITE_ONCE(last_lock_release, jiffies);
- cxt.cur_ops->writeunlock(tid);
+ if (!skip_main_lock) {
+ lock_is_write_held = false;
+ WRITE_ONCE(last_lock_release, jiffies);
+ cxt.cur_ops->writeunlock(tid);
+ }
if (cxt.cur_ops->nested_unlock)
cxt.cur_ops->nested_unlock(tid, lockset_mask);
--
2.39.1.519.gcb327c4b5f-goog
On Thu, Feb 02, 2023 at 10:34:06PM +0000, John Stultz wrote:
> In order ot extend locktorture to support lock nesting, add
> nested_lock() and nested_unlock() hooks to the torture ops.
>
> These take a 32bit lockset mask which is generated at random,
> so some number of locks will be taken before the main lock is
> taken and released afterwards.
>
> Additionally, add nlocks module parameter to allow specifying
> the number of nested locks to be used.
>
> This has been helpful to uncover issues in the proxy-exec
> series development.
>
> This was inspired by locktorture extensions originally implemented
> by Connor O'Brien, for stress testing the proxy-execution series:
> https://lore.kernel.org/lkml/[email protected]/
>
> Comments or feedback would be greatly appreciated!
I have pulled this in for testing and further review, thank you!
Should either of these files have a locktorture.nlocks=<whatever>
added?
tools/testing/selftests/rcutorture/configs/lock/LOCK02.boot
tools/testing/selftests/rcutorture/configs/lock/LOCK05.boot
The first is for mutex_lock and the second for rtmutex_lock.
This did pass a quick "torture.sh --do-none --do-locktorture", which is
good, but this uses the existing .boot files.
Thanx, Paul
> Cc: Davidlohr Bueso <[email protected]>
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Josh Triplett <[email protected]>
> Cc: Joel Fernandes <[email protected]>
> Cc: Juri Lelli <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Dietmar Eggemann <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> kernel/locking/locktorture.c | 23 +++++++++++++++++++----
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> index 9c2fb613a55d..f4fbd3194654 100644
> --- a/kernel/locking/locktorture.c
> +++ b/kernel/locking/locktorture.c
> @@ -48,6 +48,9 @@ torture_param(int, stat_interval, 60,
> torture_param(int, stutter, 5, "Number of jiffies to run/halt test, 0=disable");
> torture_param(int, verbose, 1,
> "Enable verbose debugging printk()s");
> +torture_param(int, nlocks, 0, "Number of nested locks");
> +/* Going much higher trips "BUG: MAX_LOCKDEP_CHAIN_HLOCKS too low!" errors */
> +#define MAX_LOCKS 8
>
> static char *torture_type = "spin_lock";
> module_param(torture_type, charp, 0444);
> @@ -76,10 +79,12 @@ static void lock_torture_cleanup(void);
> struct lock_torture_ops {
> void (*init)(void);
> void (*exit)(void);
> + int (*nested_lock)(int tid, u32 lockset);
> int (*writelock)(int tid);
> void (*write_delay)(struct torture_random_state *trsp);
> void (*task_boost)(struct torture_random_state *trsp);
> void (*writeunlock)(int tid);
> + void (*nested_unlock)(int tid, u32 lockset);
> int (*readlock)(int tid);
> void (*read_delay)(struct torture_random_state *trsp);
> void (*readunlock)(int tid);
> @@ -669,6 +674,7 @@ static int lock_torture_writer(void *arg)
> struct lock_stress_stats *lwsp = arg;
> int tid = lwsp - cxt.lwsa;
> DEFINE_TORTURE_RANDOM(rand);
> + u32 lockset_mask;
>
> VERBOSE_TOROUT_STRING("lock_torture_writer task started");
> set_user_nice(current, MAX_NICE);
> @@ -677,7 +683,10 @@ static int lock_torture_writer(void *arg)
> if ((torture_random(&rand) & 0xfffff) == 0)
> schedule_timeout_uninterruptible(1);
>
> + lockset_mask = torture_random(&rand);
> cxt.cur_ops->task_boost(&rand);
> + if (cxt.cur_ops->nested_lock)
> + cxt.cur_ops->nested_lock(tid, lockset_mask);
> cxt.cur_ops->writelock(tid);
> if (WARN_ON_ONCE(lock_is_write_held))
> lwsp->n_lock_fail++;
> @@ -690,6 +699,8 @@ static int lock_torture_writer(void *arg)
> lock_is_write_held = false;
> WRITE_ONCE(last_lock_release, jiffies);
> cxt.cur_ops->writeunlock(tid);
> + if (cxt.cur_ops->nested_unlock)
> + cxt.cur_ops->nested_unlock(tid, lockset_mask);
>
> stutter_wait("lock_torture_writer");
> } while (!torture_must_stop());
> @@ -830,11 +841,11 @@ lock_torture_print_module_parms(struct lock_torture_ops *cur_ops,
> const char *tag)
> {
> pr_alert("%s" TORTURE_FLAG
> - "--- %s%s: nwriters_stress=%d nreaders_stress=%d stat_interval=%d verbose=%d shuffle_interval=%d stutter=%d shutdown_secs=%d onoff_interval=%d onoff_holdoff=%d\n",
> + "--- %s%s: nwriters_stress=%d nreaders_stress=%d nlocks=%d stat_interval=%d verbose=%d shuffle_interval=%d stutter=%d shutdown_secs=%d onoff_interval=%d onoff_holdoff=%d\n",
> torture_type, tag, cxt.debug_lock ? " [debug]": "",
> - cxt.nrealwriters_stress, cxt.nrealreaders_stress, stat_interval,
> - verbose, shuffle_interval, stutter, shutdown_secs,
> - onoff_interval, onoff_holdoff);
> + cxt.nrealwriters_stress, cxt.nrealreaders_stress, nlocks,
> + stat_interval, verbose, shuffle_interval, stutter,
> + shutdown_secs, onoff_interval, onoff_holdoff);
> }
>
> static void lock_torture_cleanup(void)
> @@ -1053,6 +1064,10 @@ static int __init lock_torture_init(void)
> }
> }
>
> + /* cap nlocks to MAX_LOCKS */
> + if (nlocks > MAX_LOCKS)
> + nlocks = MAX_LOCKS;
> +
> if (cxt.cur_ops->readlock) {
> reader_tasks = kcalloc(cxt.nrealreaders_stress,
> sizeof(reader_tasks[0]),
> --
> 2.39.1.519.gcb327c4b5f-goog
>
On Thu, Feb 2, 2023 at 6:05 PM Paul E. McKenney <[email protected]> wrote:
>
> On Thu, Feb 02, 2023 at 10:34:06PM +0000, John Stultz wrote:
> > In order ot extend locktorture to support lock nesting, add
> > nested_lock() and nested_unlock() hooks to the torture ops.
> >
> > These take a 32bit lockset mask which is generated at random,
> > so some number of locks will be taken before the main lock is
> > taken and released afterwards.
> >
> > Additionally, add nlocks module parameter to allow specifying
> > the number of nested locks to be used.
> >
> > This has been helpful to uncover issues in the proxy-exec
> > series development.
> >
> > This was inspired by locktorture extensions originally implemented
> > by Connor O'Brien, for stress testing the proxy-execution series:
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > Comments or feedback would be greatly appreciated!
>
> I have pulled this in for testing and further review, thank you!
>
> Should either of these files have a locktorture.nlocks=<whatever>
> added?
>
> tools/testing/selftests/rcutorture/configs/lock/LOCK02.boot
> tools/testing/selftests/rcutorture/configs/lock/LOCK05.boot
>
> The first is for mutex_lock and the second for rtmutex_lock.
Potentially? I wasn't aware of these files. Is there some
documentation on the intent behind them?
While the nested locking is useful to cause different lock chains to
test boosting or proxy-execution, I worry they may cause extra noise
that could distract from just thrashing the *mutex lock primitive if
that's the existing intent.
So we might want additional config files for the nested case.
> This did pass a quick "torture.sh --do-none --do-locktorture", which is
> good, but this uses the existing .boot files.
Yeah, probably no effective change in that case.
thanks
-john
On Thu, Feb 02, 2023 at 07:56:05PM -0800, John Stultz wrote:
> On Thu, Feb 2, 2023 at 6:05 PM Paul E. McKenney <[email protected]> wrote:
> >
> > On Thu, Feb 02, 2023 at 10:34:06PM +0000, John Stultz wrote:
> > > In order ot extend locktorture to support lock nesting, add
> > > nested_lock() and nested_unlock() hooks to the torture ops.
> > >
> > > These take a 32bit lockset mask which is generated at random,
> > > so some number of locks will be taken before the main lock is
> > > taken and released afterwards.
> > >
> > > Additionally, add nlocks module parameter to allow specifying
> > > the number of nested locks to be used.
> > >
> > > This has been helpful to uncover issues in the proxy-exec
> > > series development.
> > >
> > > This was inspired by locktorture extensions originally implemented
> > > by Connor O'Brien, for stress testing the proxy-execution series:
> > > https://lore.kernel.org/lkml/[email protected]/
> > >
> > > Comments or feedback would be greatly appreciated!
> >
> > I have pulled this in for testing and further review, thank you!
> >
> > Should either of these files have a locktorture.nlocks=<whatever>
> > added?
> >
> > tools/testing/selftests/rcutorture/configs/lock/LOCK02.boot
> > tools/testing/selftests/rcutorture/configs/lock/LOCK05.boot
> >
> > The first is for mutex_lock and the second for rtmutex_lock.
>
> Potentially? I wasn't aware of these files. Is there some
> documentation on the intent behind them?
There is a LOCK02 file that contains Kconfig options and the LOCK02.boot
file contains kernel boot parameters. There is a CFLIST file that
contains the list of such files that the command below will test by
default.
The best documentation is probably here:
https://paulmck.livejournal.com/57769.html
https://paulmck.livejournal.com/58077.html
> While the nested locking is useful to cause different lock chains to
> test boosting or proxy-execution, I worry they may cause extra noise
> that could distract from just thrashing the *mutex lock primitive if
> that's the existing intent.
The intent is to find bugs by whatever means necessary, within reason.
> So we might want additional config files for the nested case.
That would work.
> > This did pass a quick "torture.sh --do-none --do-locktorture", which is
> > good, but this uses the existing .boot files.
>
> Yeah, probably no effective change in that case.
At least nothing else got broken. ;-)
Thanx, Paul