Re: [PATCH tip/core/rcu 07/15] rcu: Add event tracing to ->gp_tasks update at GP start

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Thu, Jul 27, 2017 at 09:38:10PM -0400, Steven Rostedt wrote:
> On Mon, 24 Jul 2017 14:44:36 -0700
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > There is currently event tracing to track when a task is preempted
> > within a preemptible RCU read-side critical section, and also when that
> > task subsequently reaches its outermost rcu_read_unlock(), but none
> > indicating when a new grace period starts when that grace period must
> > wait on pre-existing readers that have been been preempted at least once
> > since the beginning of their current RCU read-side critical sections.
> > 
> > This commit therefore adds an event trace at grace-period start in
> > the case where there are such readers.  Note that only the first
> > reader in the list is traced.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  kernel/rcu/tree_plugin.h | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index 14ba496a13cd..3e3f92e981a1 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -636,10 +636,17 @@ static int rcu_print_task_exp_stall(struct rcu_node *rnp)
> >   */
> >  static void rcu_preempt_check_blocked_tasks(struct rcu_node *rnp)
> >  {
> > +	struct task_struct *t;
> > +
> >  	RCU_LOCKDEP_WARN(preemptible(), "rcu_preempt_check_blocked_tasks() invoked with preemption enabled!!!\n");
> >  	WARN_ON_ONCE(rcu_preempt_blocked_readers_cgp(rnp));
> > -	if (rcu_preempt_has_tasks(rnp))
> > +	if (rcu_preempt_has_tasks(rnp)) {
> 
> The only function of this if block is to fill the content of the
> trace event, correct?
> 
> What about doing:
> 
> 	if (trace_rcu_unlock_preempted_task_enabled() &&
> 	    rcu_preempt_has_tasks(rnp)) {
> 
> instead? The trace_rcu_unlock_preempted_task_enabled() is a static
> branch (aka jump_label), which would make the above a constant branch
> when tracing is not enabled, and would keep this from adding any extra
> overhead.
> 
> -- Steve
> 
> >  		rnp->gp_tasks = rnp->blkd_tasks.next;

The trace_rcu_unlock_preempted_task_enabled() call is a new one on me,
thank you!

Unfortunately, the above assignment to rnp->gp_tasks is required even
if tracing is disabled.  The reason is that the newly started grace
period needs to wait on all tasks that have been preempted within their
current RCU read-side critical section, and rnp->gp_tasks records the
point in the rnp->blkd_tasks list beyond which all preempted tasks block
this new grace period.

If this assignment is omitted, we get too-short grace periods, and the
tasks on this list might still be holding references to stuff that gets
freed at the end of this new grace period.

I applied your two acks, thank you!

							Thanx, Paul

> > +		t = container_of(rnp->gp_tasks, struct task_struct,
> > +				 rcu_node_entry);
> > +		trace_rcu_unlock_preempted_task(TPS("rcu_preempt-GPS"),
> > +						rnp->gpnum, t->pid);
> > +	}
> >  	WARN_ON_ONCE(rnp->qsmask);
> >  }
> >  
>