Re: [PATCH v2] rcu-tasks: Make RCU Tasks Trace checking for userspace execution

["Paul E. McKenney" <paulmck@kernel.org>]

On Mon, Jul 18, 2022 at 11:54:53PM +0000, Zhang, Qiang1 wrote:
> On Mon, Jul 18, 2022 at 08:16:10AM +0800, Zqiang wrote:
> > For RCU tasks trace, the userspace execution is also a valid quiescent
> > state, if the task is in userspace, the ->trc_reader_nesting should be
> > zero and if the ->trc_reader_special.b.need_qs is not set, set the
> > tasks ->trc_reader_special.b.need_qs is TRC_NEED_QS_CHECKED, this cause
> > grace-period kthread remove it from holdout list if it remains here.
> > 
> > This commit add rcu_tasks_trace_qs() to rcu_flavor_sched_clock_irq()
> > when the kernel built with no PREEMPT_RCU.
> > 
> > Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> >
> >The looks plausible to me, but can you tell me how this avoids the
> >following sequence of events?
> >
> >o	CPU 0 takes a scheduling-clock interrupt.  Just before this
> >	point CPU 0 was running in user context, thus as you say
> >	should not be in an RCU Tasks quiescent state.
> >
> >o	CPU 0 enters an RCU Tasks Trace read-side critical section.
>                
>  if I understand correctly, you mean that CPU0 enters an RCU Tasks Trace
>  read-side critical section in scheduling-clock interrupt context.

Exactly, as might happen if one of the functions in the scheduling-clock
interrupt hander were traced/instrumented.

> >o	CPU 1 starts a new RCU Tasks Trace grace period.
> 
> The grace period kthread will scan running tasks on each CPU, 
> The tasks currently running on CPU0 will be recorded in the holdout list.

Yes, very good.

> >o	CPU 0 reaches the newly added rcu_note_voluntary_context_switch().
> 
> In this time, if CPU0 still in RCU Tasks Trace read-side critical section, the tasks
> which running on CPU0 will insert CPU0 blocked list. when this tasks exit 
> RCU Tasks Trace read-side critical section, this task will remove from CPU0 block list.
> 
> Did I understand the scenario described above correctly?

Looks like it to me.

Could you please resend the patch with this explained in the commit
log?  Possibly for the benefit of your future self.  ;-)

							Thanx, Paul

> Thanks
> Zqiang
> 
> >
> >	Except that the quiescent state implied by userspace execution
> >	was before the new grace period, and thus does not apply to it.
> >
> >(Yes, I know, if this is a bug in this patch, the bug already exists
> >due to the call in rcu_flavor_sched_clock_irq() for !PREEMPT kernels,
> >but if this change is safe, it should be possible to explain why.)
> >
> >							Thanx, Paul
> >
> > ---
> >  v1->v2:
> >  Fix build error due to undeclared rcu_tasks_trace_qs(), note in no-PREEMPT_RCU
> >  kernel, the RCU Tasks is replaced by RCU, so rcu_note_voluntary_context_switch()
> >  only include rcu_tasks_trace_qs().
> > 
> >  kernel/rcu/tree_plugin.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index 4152816dd29f..5fb0b2dd24fd 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -976,7 +976,7 @@ static void rcu_flavor_sched_clock_irq(int user)
> >  		 * neither access nor modify, at least not while the
> >  		 * corresponding CPU is online.
> >  		 */
> > -
> > +		rcu_note_voluntary_context_switch(current);
> >  		rcu_qs();
> >  	}
> >  }
> > -- 
> > 2.25.1
> >