Re: [PATCH v2 tip/core/rcu 04/39] srcu: Check for tardy grace-period activity in cleanup_srcu_struct()

[Josh Triplett]

On Mon, Apr 17, 2017 at 04:44:51PM -0700, Paul E. McKenney wrote:
> Users of SRCU are obliged to complete all grace-period activity before
> invoking cleanup_srcu_struct().  This means that all calls to either
> synchronize_srcu() or synchronize_srcu_expedited() must have returned,
> and all calls to call_srcu() must have returned, and the last call to
> call_srcu() must have been followed by a call to srcu_barrier().
> Furthermore, the caller must have done something to prevent any
> further calls to synchronize_srcu(), synchronize_srcu_expedited(),
> and call_srcu().
> 
> Therefore, if there has ever been an invocation of call_srcu() on
> the srcu_struct in question, the sequence of events must be as
> follows:
> 
> 1.  Prevent any further calls to call_srcu().
> 2.  Wait for any pre-existing call_srcu() invocations to return.
> 3.  Invoke srcu_barrier().
> 4.  It is now safe to invoke cleanup_srcu_struct().
> 
> On the other hand, if there has ever been a call to synchronize_srcu()
> or synchronize_srcu_expedited(), the sequence of events must be as
> follows:
> 
> 1.  Prevent any further calls to synchronize_srcu() or
>     synchronize_srcu_expedited().
> 2.  Wait for any pre-existing synchronize_srcu() or
>     synchronize_srcu_expedited() invocations to return.
> 3.  It is now safe to invoke cleanup_srcu_struct().
> 
> If there have been calls to all both types of functions (call_srcu()
> and either of synchronize_srcu() and synchronize_srcu_expedited()), then
> the caller must do the first three steps of the call_srcu() procedure
> above and the first two steps of the synchronize_s*() procedure above,
> and only then invoke cleanup_srcu_struct().

This commit message clearly explains the correct sequence for the
client, but not which aspects of this the change now enforces.  Some of
the steps above remain the responsibility of the caller, while the
callee now checks more of them.  Could you add something at the end
explaining the change and what it enforces?

> Reported-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>

With the above change:
Reviewed-by: Josh Triplett <josh@xxxxxxxxxxxxxxxx>

>  kernel/rcu/srcu.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/rcu/srcu.c b/kernel/rcu/srcu.c
> index ba41a5d04b49..6beeba7b0b67 100644
> --- a/kernel/rcu/srcu.c
> +++ b/kernel/rcu/srcu.c
> @@ -261,6 +261,11 @@ void cleanup_srcu_struct(struct srcu_struct *sp)
>  {
>  	if (WARN_ON(srcu_readers_active(sp)))
>  		return; /* Leakage unless caller handles error. */
> +	if (WARN_ON(!rcu_all_batches_empty(sp)))
> +		return; /* Leakage unless caller handles error. */
> +	flush_delayed_work(&sp->work);
> +	if (WARN_ON(sp->running))
> +		return; /* Caller forgot to stop doing call_srcu()? */
>  	free_percpu(sp->per_cpu_ref);
>  	sp->per_cpu_ref = NULL;
>  }
> -- 
> 2.5.2
>