On Thu, 27 Jul 2017 20:22:32 -0700
"Paul E. McKenney" <paul...@linux.vnet.ibm.com> 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" <paul...@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 <paul...@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!
>
And with you answer about the block not just being for tracing, you can
add my acked-by here too ;-)
-- Steve
