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

[Steven Rostedt]

On Thu, 27 Jul 2017 20:22:32 -0700
"Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx> wrote:

> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >  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!
> 

And with you answer about the block not just being for tracing, you can
add my acked-by here too ;-)

-- Steve