On Tue, Mar 05, 2024 at 07:57:40PM +0000, Mark Rutland wrote:
> On Tue, Mar 05, 2024 at 09:53:42AM -0800, Paul E. McKenney wrote:
> > On Mon, Mar 04, 2024 at 04:16:01AM -0500, Joel Fernandes wrote:
> > > Hi Paul,
> >
> > Thank you, Joel!
> >
> > > On 3/2/2024 8:01 PM, Joel Fernandes wrote:
> > > >> As you noted, one thing that Ankur's series changes is that preemption
> > > >> can occur anywhere that it is not specifically disabled in kernels
> > > >> built with CONFIG_PREEMPT_NONE=y or CONFIG_PREEMPT_VOLUNTARY=y. This
> > > >> in
> > > >> turn changes Tasks Rude RCU's definition of a quiescent state for these
> > > >> kernels, adding all code regions where preemption is not specifically
> > > >> disabled to the list of such quiescent states.
> > > >>
> > > >> Although from what I know, this is OK, it would be good to check the
> > > >> calls to call_rcu_tasks_rude() or synchronize_rcu_tasks_rude() are set
> > > >> up so as to expect these new quiescent states. One example where it
> > > >> would definitely be OK is if there was a call to
> > > >> synchronize_rcu_tasks()
> > > >> right before or after that call to synchronize_rcu_tasks_rude().
> > > >>
> > > >> Would you be willing to check the call sites to verify that they
> > > >> are OK with this change in
> > > > Yes, I will analyze and make sure those users did not unexpectedly
> > > > assume something about AUTO (i.e. preempt enabled sections using
> > > > readers).
> > >
> > > Other than RCU test code, there are just 3 call sites for RUDE right now,
> > > all in
> > > ftrace.c.
> > >
> > > (Long story short, PREEMPT_AUTO should not cause wreckage in
> > > TASKS_RCU_RUDE
> > > other than any preexisting wreckage that !PREEMPT_AUTO already had. Steve
> > > is on
> > > CC as well to CMIIW).
> > >
> > > Case 1: For !CONFIG_DYNAMIC_FTRACE update of ftrace_trace_function
> > >
> > > This config is itself expected to be slow. However seeing what it does,
> > > it is
> > > trying to make sure the global function pointer "ftrace_trace_function" is
> > > updated and any readers of that pointers would have finished reading it.
> > > I don't
> > > personally think preemption has to be disabled across the entirety of the
> > > section that calls into this function. So sensitivity to preempt disabling
> > > should not be relevant for this case IMO, but lets see if ftrace folks
> > > disagree
> > > (on CC). It has more to do with, any callers of this function pointer are
> > > no
> > > longer calling into the old function.
> >
> > Assuming the loads from the function pointer aren't torn by the compiler,
> > they will be loaded by a single instruction, which as you say cannot
> > be preempted. Might be good to have READ_ONCE() if they aren't already
> > in place.
>
> As a heads-up I'm actively digging through case 1 now and I think the existing
> code is actually redundant or broken depending on architecture and
> configuration (but largely redundant, hence not seeing any reports of an
> issue).
>
> I've dug through v3.14 up to v5.4, and I'll hopefully have a writeup of that
> out tomorrow, or in the next couple of hours if I continue after dinner...
>
> I haven't yet looked at cases 2 or 3 yet, and I haven't convinced myself on
> how
> the CONFIG_DYNAMIC_FTRACE=y case works either.
Ouch! Looking forward to seeing what you come up with.
Thanx, Paul
> Mark.
>
> > > Case 2: Trampoline structures accessing
> > >
> > > For this there is a code comment that says preemption will disabled so it
> > > should
> > > not be dependent on any of the preemptiblity modes, because
> > > preempt_disable()
> > > should disable preempt with PREEMPT_AUTO.
> > >
> > > /*
> > > * We need to do a hard force of sched synchronization.
> > > * This is because we use preempt_disable() to do RCU, but
> > > * the function tracers can be called where RCU is not watching
> > > * (like before user_exit()). We can not rely on the RCU
> > > * infrastructure to do the synchronization, thus we must do it
> > > * ourselves.
> > > */
> > > synchronize_rcu_tasks_rude();
> > > [...]
> > > ftrace_trampoline_free(ops);
> > >
> > > Code comment probably needs update because it says 'can not rely on
> > > RCU..' ;-)
> >
> > My guess is that this comment is left over from when that call to
> > synchronize_rcu_tasks_rude() was open-coded. ;-)
> >
> > Maybe "We can not rely on vanilla RCU to do..."?
> >
> > > My *guess* is the preempt_disable() mentioned in this case is
> > > ftrace_ops_trampoline() where trampoline-related datas tructures are
> > > accessed
> > > for stack unwinding purposes. This is a data structure protection thing
> > > AFAICS
> > > and nothing to do with "trampoline execution" itself which needs "Tasks
> > > RCU" to
> > > allow for preemption in trampolines.
> >
> > Sounds plausible to me, but let's see what Steve's thoughts are.
> >
> > > Case 3: This has to do with update of function graph tracing and there is
> > > the
> > > same comment as case 2, where preempt will be disabled in readers, so it
> > > should
> > > be safe for PREEMPT_AUTO (famous last words).
> > >
> > > Though I am not yet able to locate that preempt_disable() which is not an
> > > PREEMPT_AUTO-related issue anyway. Maybe its buried in function graph
> > > tracing
> > > logic somewhere?
> >
> > With the trampolines, isn't synchronize_rcu_tasks_rude() paired with
> > a call to synchronize_rcu_tasks()? In that case, rude's only job is
> > getting all CPUs out of their previous sojourn in either the entry/exit
> > code or the deep idle loop. RCU Tasks waits for each task to voluntarily
> > block, which takes care of all tasks executing elsewhere. (Recall that
> > RCU Tasks ignores the idle tasks.)
> >
> > > Finally, my thought also was, if any of these thread usages/cases of
> > > Tasks RCU
> > > RUDE assume working only on a CONFIG_PREEMPT_NONE=y or
> > > CONFIG_PREEMPT_VOLUNTARY=y kernel, that could be worrying but AFAICS,
> > > they don't
> > > assume anything related to that.
> >
> > Good point, most generic code should need to tolerate preemption in
> > any case. But I have nine commits queued thus far that handle some
> > CONFIG_AUTO breakage or another, so a little paranoia won't go too
> > far amiss. ;-)
> >
> > Remaining on my list are uses of the various CONFIG_PREEMPT* Kconfig
> > options, both in code and in Makefiles. Though who knows? Perhaps Ankur
> > or Thomas have already done that.
> >
> > Thanx, Paul
> >
