Hi Paul,
> On 2020/9/23 7:24, Paul E. McKenney wrote:
snip
>> Fix it by adding an exit hook in lock_torture_ops and
>> use it to call percpu_free_rwsem() for percpu rwsem torture
>> before the module is removed, so we can ensure rcu_sync_func()
>> completes before module exits.
>>
>> Also needs to call exit hook if lock_torture_init() fails half-way,
>> so use ctx->cur_ops != NULL to signal that init hook has been called.
>
> Good catch, but please see below for comments and questions.
>
>> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
>> ---
>> kernel/locking/locktorture.c | 28 ++++++++++++++++++++++------
>> 1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c
>> index bebdf98e6cd78..e91033e9b6f95 100644
>> --- a/kernel/locking/locktorture.c
>> +++ b/kernel/locking/locktorture.c
>> @@ -74,6 +74,7 @@ static void lock_torture_cleanup(void);
>> */
>> struct lock_torture_ops {
>> void (*init)(void);
>> + void (*exit)(void);
>
> This is fine, but why not also add a flag to the lock_torture_cxt
> structure that is set when the ->init() function is called? Perhaps
> something like this in lock_torture_init():
>
> if (cxt.cur_ops->init) {
> cxt.cur_ops->init();
> cxt.initcalled = true;
> }
>
You are right. Add a new field to indicate the init hook has been
called is much better than reusing ctx->cur_ops != NULL to do that.
>> int (*writelock)(void);
>> void (*write_delay)(struct torture_random_state *trsp);
>> void (*task_boost)(struct torture_random_state *trsp);
>> @@ -571,6 +572,11 @@ void torture_percpu_rwsem_init(void)
>> BUG_ON(percpu_init_rwsem(&pcpu_rwsem));
>> }
>>
>> +static void torture_percpu_rwsem_exit(void)
>> +{
>> + percpu_free_rwsem(&pcpu_rwsem);
>> +}
>> +
snip
>> @@ -828,6 +836,12 @@ static void lock_torture_cleanup(void)
>> cxt.lrsa = NULL;
>>
>> end:
>> + /* If init() has been called, then do exit() accordingly */
>> + if (cxt.cur_ops) {
>> + if (cxt.cur_ops->exit)
>> + cxt.cur_ops->exit();
>> + cxt.cur_ops = NULL;
>> + }
>
> The above can then be:
>
> if (cxt.initcalled && cxt.cur_ops->exit)
> cxt.cur_ops->exit();
>
> Maybe you also need to clear cxt.initcalled at this point, but I don't
> immediately see why that would be needed.
>
Because we are doing cleanup, so I think reset initcalled to false is OK
after the cleanup is done.
>> torture_cleanup_end();
>> }
>>
>> @@ -835,6 +849,7 @@ static int __init lock_torture_init(void)
>> {
>> int i, j;
>> int firsterr = 0;
>> + struct lock_torture_ops *cur_ops;
>
> And then you don't need this extra pointer. Not that this pointer is bad
> in and of itself, but using (!cxt.cur_ops) to indicate that the ->init()
> function has not been called is an accident waiting to happen.
>
> And the changes below are no longer needed.
>
> Or am I missing something subtle?
>
Thanks for your suggestion. Will send v2.
Thanks.
