2020-09-17 14:19:12

by Hou Tao

[permalink] [raw]
Subject: [PATCH 1/2] locktorture: doesn't check nreaders_stress when no readlock support

To ensure there is always at least one locking thread.

Signed-off-by: Hou Tao <[email protected]>
---
kernel/locking/locktorture.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index 9cfa5e89cff7f..bebdf98e6cd78 100644
--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -868,7 +868,8 @@ static int __init lock_torture_init(void)
goto unwind;
}

- if (nwriters_stress == 0 && nreaders_stress == 0) {
+ if (nwriters_stress == 0 &&
+ (!cxt.cur_ops->readlock || nreaders_stress == 0)) {
pr_alert("lock-torture: must run at least one locking thread\n");
firsterr = -EINVAL;
goto unwind;
--
2.25.0.4.g0ad7144999


2020-09-17 17:01:24

by Paul E. McKenney

[permalink] [raw]
Subject: Re: [PATCH 1/2] locktorture: doesn't check nreaders_stress when no readlock support

On Thu, Sep 17, 2020 at 09:59:09PM +0800, Hou Tao wrote:
> To ensure there is always at least one locking thread.
>
> Signed-off-by: Hou Tao <[email protected]>
> ---
> kernel/locking/locktorture.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> index 9cfa5e89cff7f..bebdf98e6cd78 100644
> --- a/kernel/locking/locktorture.c
> +++ b/kernel/locking/locktorture.c
> @@ -868,7 +868,8 @@ static int __init lock_torture_init(void)
> goto unwind;
> }
>
> - if (nwriters_stress == 0 && nreaders_stress == 0) {
> + if (nwriters_stress == 0 &&
> + (!cxt.cur_ops->readlock || nreaders_stress == 0)) {

You lost me on this one. How does it help to allow tests with zero
writers on exclusive locks? Or am I missing something subtle here?

Thanx, Paul

> pr_alert("lock-torture: must run at least one locking thread\n");
> firsterr = -EINVAL;
> goto unwind;
> --
> 2.25.0.4.g0ad7144999
>

2020-09-18 01:14:57

by Hou Tao

[permalink] [raw]
Subject: Re: [PATCH 1/2] locktorture: doesn't check nreaders_stress when no readlock support

Hi Paul,

On 2020/9/18 0:58, Paul E. McKenney wrote:
> On Thu, Sep 17, 2020 at 09:59:09PM +0800, Hou Tao wrote:
>> To ensure there is always at least one locking thread.
>>
>> Signed-off-by: Hou Tao <[email protected]>
>> ---
>> kernel/locking/locktorture.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
>> index 9cfa5e89cff7f..bebdf98e6cd78 100644
>> --- a/kernel/locking/locktorture.c
>> +++ b/kernel/locking/locktorture.c
>> @@ -868,7 +868,8 @@ static int __init lock_torture_init(void)
>> goto unwind;
>> }
>>
>> - if (nwriters_stress == 0 && nreaders_stress == 0) {
>> + if (nwriters_stress == 0 &&
>> + (!cxt.cur_ops->readlock || nreaders_stress == 0)) {
>
> You lost me on this one. How does it help to allow tests with zero
> writers on exclusive locks? Or am I missing something subtle here?
>
The purpose is to prohibit test with only readers on exclusive locks, not allow it.

So if the module parameters are "torture_type=mutex_lock nwriters_stress=0 nreaders_stress=3",
locktorture can fail early instead of continuing but doing nothing useful.

Regards,
Tao

> Thanx, Paul
>
>> pr_alert("lock-torture: must run at least one locking thread\n");
>> firsterr = -EINVAL;
>> goto unwind;
>> --
>> 2.25.0.4.g0ad7144999
>>
> .
>

2020-09-18 03:39:40

by Paul E. McKenney

[permalink] [raw]
Subject: Re: [PATCH 1/2] locktorture: doesn't check nreaders_stress when no readlock support

On Fri, Sep 18, 2020 at 09:13:14AM +0800, Hou Tao wrote:
> Hi Paul,
>
> On 2020/9/18 0:58, Paul E. McKenney wrote:
> > On Thu, Sep 17, 2020 at 09:59:09PM +0800, Hou Tao wrote:
> >> To ensure there is always at least one locking thread.
> >>
> >> Signed-off-by: Hou Tao <[email protected]>
> >> ---
> >> kernel/locking/locktorture.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> >> index 9cfa5e89cff7f..bebdf98e6cd78 100644
> >> --- a/kernel/locking/locktorture.c
> >> +++ b/kernel/locking/locktorture.c
> >> @@ -868,7 +868,8 @@ static int __init lock_torture_init(void)
> >> goto unwind;
> >> }
> >>
> >> - if (nwriters_stress == 0 && nreaders_stress == 0) {
> >> + if (nwriters_stress == 0 &&
> >> + (!cxt.cur_ops->readlock || nreaders_stress == 0)) {
> >
> > You lost me on this one. How does it help to allow tests with zero
> > writers on exclusive locks? Or am I missing something subtle here?
> >
> The purpose is to prohibit test with only readers on exclusive locks, not allow it.
>
> So if the module parameters are "torture_type=mutex_lock nwriters_stress=0 nreaders_stress=3",
> locktorture can fail early instead of continuing but doing nothing useful.

Very good!

Now please make that clear in the commit log. (Your English looks to
me to be more than equal to that challenge.)

In this commit log, please first state what is wrong. Then what the
change is and how it improves things.

Thanx, Paul

> Regards,
> Tao
>
> > Thanx, Paul
> >
> >> pr_alert("lock-torture: must run at least one locking thread\n");
> >> firsterr = -EINVAL;
> >> goto unwind;
> >> --
> >> 2.25.0.4.g0ad7144999
> >>
> > .
> >

2020-09-18 11:39:00

by Hou Tao

[permalink] [raw]
Subject: [PATCH v2 1/2] locktorture: doesn't check nreaders_stress when no readlock support

When do locktorture for exclusive lock which doesn't have readlock
support, the following module parameters will be considered as valid:

torture_type=mutex_lock nwriters_stress=0 nreaders_stress=1

But locktorture will do nothing useful, so instead of permitting
these useless parameters, let's reject these parameters by returning
-EINVAL during module init.

Signed-off-by: Hou Tao <[email protected]>
---
kernel/locking/locktorture.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index 9cfa5e89cff7f..bebdf98e6cd78 100644
--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -868,7 +868,8 @@ static int __init lock_torture_init(void)
goto unwind;
}

- if (nwriters_stress == 0 && nreaders_stress == 0) {
+ if (nwriters_stress == 0 &&
+ (!cxt.cur_ops->readlock || nreaders_stress == 0)) {
pr_alert("lock-torture: must run at least one locking thread\n");
firsterr = -EINVAL;
goto unwind;
--
2.25.0.4.g0ad7144999

2020-09-18 18:03:02

by Paul E. McKenney

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] locktorture: doesn't check nreaders_stress when no readlock support

On Fri, Sep 18, 2020 at 07:44:24PM +0800, Hou Tao wrote:
> When do locktorture for exclusive lock which doesn't have readlock
> support, the following module parameters will be considered as valid:
>
> torture_type=mutex_lock nwriters_stress=0 nreaders_stress=1
>
> But locktorture will do nothing useful, so instead of permitting
> these useless parameters, let's reject these parameters by returning
> -EINVAL during module init.
>
> Signed-off-by: Hou Tao <[email protected]>

Much better, much easier for people a year from now to understand.
Queued for v5.11, thank you!

I did edit the commit log a bit as shown below, so please let me
know if I messed anything up.

Thanx, Paul

commit 4985c52e3b5237666265e59f56856f485ee36e71
Author: Hou Tao <[email protected]>
Date: Fri Sep 18 19:44:24 2020 +0800

locktorture: Ignore nreaders_stress if no readlock support

Exclusive locks do not have readlock support, which means that a
locktorture run with the following module parameters will do nothing:

torture_type=mutex_lock nwriters_stress=0 nreaders_stress=1

This commit therefore rejects this combination for exclusive locks by
returning -EINVAL during module init.

Signed-off-by: Hou Tao <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>

diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
index 316531d..046ea2d 100644
--- a/kernel/locking/locktorture.c
+++ b/kernel/locking/locktorture.c
@@ -870,7 +870,8 @@ static int __init lock_torture_init(void)
goto unwind;
}

- if (nwriters_stress == 0 && nreaders_stress == 0) {
+ if (nwriters_stress == 0 &&
+ (!cxt.cur_ops->readlock || nreaders_stress == 0)) {
pr_alert("lock-torture: must run at least one locking thread\n");
firsterr = -EINVAL;
goto unwind;

2020-09-19 03:29:20

by Hou Tao

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] locktorture: doesn't check nreaders_stress when no readlock support

Hi Paul,

On 2020/9/19 1:59, Paul E. McKenney wrote:
> On Fri, Sep 18, 2020 at 07:44:24PM +0800, Hou Tao wrote:
>> When do locktorture for exclusive lock which doesn't have readlock
>> support, the following module parameters will be considered as valid:
>>
>> torture_type=mutex_lock nwriters_stress=0 nreaders_stress=1
>>
>> But locktorture will do nothing useful, so instead of permitting
>> these useless parameters, let's reject these parameters by returning
>> -EINVAL during module init.
>>
>> Signed-off-by: Hou Tao <[email protected]>
>
> Much better, much easier for people a year from now to understand.
> Queued for v5.11, thank you!
>
> I did edit the commit log a bit as shown below, so please let me
> know if I messed anything up.
>
Thanks for your edit, it looks more clearer.

Regards,
Tao
> Thanx, Paul
>
> commit 4985c52e3b5237666265e59f56856f485ee36e71
> Author: Hou Tao <[email protected]>
> Date: Fri Sep 18 19:44:24 2020 +0800
>
> locktorture: Ignore nreaders_stress if no readlock support
>
> Exclusive locks do not have readlock support, which means that a
> locktorture run with the following module parameters will do nothing:
>
> torture_type=mutex_lock nwriters_stress=0 nreaders_stress=1
>
> This commit therefore rejects this combination for exclusive locks by
> returning -EINVAL during module init.
>
> Signed-off-by: Hou Tao <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
>
> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
> index 316531d..046ea2d 100644
> --- a/kernel/locking/locktorture.c
> +++ b/kernel/locking/locktorture.c
> @@ -870,7 +870,8 @@ static int __init lock_torture_init(void)
> goto unwind;
> }
>
> - if (nwriters_stress == 0 && nreaders_stress == 0) {
> + if (nwriters_stress == 0 &&
> + (!cxt.cur_ops->readlock || nreaders_stress == 0)) {
> pr_alert("lock-torture: must run at least one locking thread\n");
> firsterr = -EINVAL;
> goto unwind;
> .
>