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

[Paul E. McKenney]

On Wed, Sep 23, 2020 at 10:24:20AM +0800, Hou Tao wrote:
> 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.

Maybe best to try it both ways and see how each really works?

We might each have our opinions, but the computer's opinion is the one
that really counts.  ;-)

> >>  	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.

Looking forward to seeing it!

							Thanx, Paul