Re: [PATCH 2/2] locktorture: call percpu_free_rwsem() to do percpu-rwsem cleanup

[Hou Tao]

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.