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

[Paul E. McKenney]

On Fri, Jul 28, 2017 at 08:18:18AM -0400, Steven Rostedt wrote:
> 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 ;-)

Added, thank you!

							Thanx, Paul