Re: [GIT PULL] Generic entry for ARM

[Steven Rostedt]

On Mon, 7 Apr 2025 16:47:05 -0700
Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:

> noinstr is much broader than notrace.  No instrumentation of any kind
> (ftrace, kprobes, sanitizers, profilers, etc), in the prologue or body
> of the function or its callees, except for regions marked by
> instrumentation_{begin,end}().
> 
> noinstr is needed in early/late entry code before/after RCU transitions,
> as instrumentation code might depend on RCU.  Also, entry code tends to
> be sensitive and unable to handle random instrumentation code.

From my understanding, noinstr is usually used in context switching code.
That is, switching from user to kernel and back. It's highly sensitive
and I believe there's CPU helpers that will prevent random code from
happening here (I'm guessing at this, but from the strictness they have
about not doing any instrumentation and how things will break if you do, I
feel that there's going to be a hardware enforcement here, if it's not
already there).

> 
> notrace is more narrow, it just means "no function tracing".  It's used
> by tracing code and the functions it calls.  It's also used for early
> boot code, as ftrace can be enabled on the kernel cmdline.

Right, notrace is basically code that doesn't make sense to trace. Like the
function tracing code itself. Also clocks that the function tracing uses.
You really don't want the clocks being traced as they are used by the
function tracer. Now that we have ftrace_test_recursion_trylock(), it
doesn't crash like it use to. But recursion does cause overhead.

Note, I've been thinking of letting some of the tracing code be traced.
Like the access to files and such. Just because there's been times I want
to know what code is actually being called there (like when I set up a new
histogram!).

> 
> I believe noinstr is not typically needed for boot code, at least for
> the primary CPU.  RCU isn't present yet, and many instrumentations might
> not yet be available.  Or their handlers can detect whether they've been
> initialized yet.  Or they're disabled in .init sections.
> 
> Whether noinstr is needed for *secondary* boot code, I don't know.  It
> may be dependent on the instrumentation implementations, e.g., whether
> they're enabled globally or per-CPU.  At least on x86, start_secondary()
> is notrace.  I don't know enough to say whether that's sufficient.

Not sure, as there's really no context switch at this time. But Peter and
Thomas would know better than I here (continuing to pass the buck!).

> 
> > > sched_clock_noinstr() is tagged noinstr and sched_clock() is
> > > tagged notrace, so there must be a difference here.

sched_clock() is using in tracing. If it wasn't tagged notrace, the
function tracer would be recursing on it.

The noinstr looks like it was created just because some archs call
sched_clock in noinstr code?

> > > 
> > > secondary_start_kernel() is tagged "notrace" on arm64 but
> > > not on arm, should it be tagged "noinstr" or "notrace"?

According to commit b154886f78924:

    arm64: make secondary_start_kernel() notrace
    
    We can't call function trace hook before setup percpu offset.
    When entering secondary_start_kernel(), percpu offset has not
    been initialized.  So this lead hotplug malfunction.
    Here is the flow to reproduce this bug:
    
    echo 0 > /sys/devices/system/cpu/cpu1/online
    echo function > /sys/kernel/debug/tracing/current_tracer
    echo 1 > /sys/kernel/debug/tracing/tracing_on
    echo 1 > /sys/devices/system/cpu/cpu1/online

If other archs crash when doing the above, then sure. Label it notrace ;-)


> > > 
> > > This kind of stuff makes me uncertain about how this is to be
> > > done. A "noinstr vs notrace" section in Documentation/core-api/entry.rst
> > > would help a lot I think!  
> 
> Sounds like a good idea.  We just need a volunteer :-)
> 

Agreed (but not me!)

-- Steve