Re: [RFC PATCH] ftrace: Fix lost recursion records in ftrace_record_recursion()

[Andrea Parri <parri.andrea@gmail.com>]

On Wed, Oct 14, 2026 at 05:28:21PM -0400, Steven Rostedt wrote:
> > Can't claim to fully understand the logic behind ftrace_record_recursion().
> > Notably, its smp_mb__after_atomic() calls and the lack of barrier comments
> > both look suspicious to my LKMM-trained eyes.  ;)  Hence the RFC.
>
> Well, the nr_records is updated via a cmpxchg() which itself is a full
> memory barrier. The smp_mb__after_atomic() calls are to make sure the value
> read is in sync with other CPUs before doing the compares. It also syncs
> with the clearing function that is disabling and the writes, clears it, and
> enables it (the -1 updates).
>
> Otherwise, IIUC, the atomic_read() itself doesn't guarantee that what you
> read is what is in memory without explicitly using the smp_mb__after_atomic().

Thanks for the explanation.  I don't think smp_mb__after_atomic() gives
us that, though.  Per Documentation/atomic_t.txt, the barriers
smp_mb__{before,after}_atomic() "only apply to the RMW atomic ops";
atomic_read() and atomic_set() are not RMW ops, so after them the
barrier is not guaranteed to order anything.  Most architectures
implement it as smp_mb(), which happens to cover these uses, but on x86
it is not even a compiler barrier, so the compiler is free to move the
memset() before atomic_set(-1); LoongArch still defines it as
barrier(), though a fix for that is pending [1].
And no barrier makes a load return a more recent value; barriers only
order accesses against each other.

What I think the code wants is: on the recording side, the load from
nr_records ordered before the loads from recursed_functions[], e.g.
atomic_read_acquire(); on the clearing side, the store of -1 ordered
before the memset(), for which smp_wmb() is enough.  (The memset()
itself still races with the recorders.  Marking its stores would make
that a well-defined race, but clearing would stay best-effort, as the
existing comment says, unless the clear waits for in-flight recorders,
e.g. with a grace period.)

This is independent of the cached_function bug, so I'll leave it out of
v1 and send it as a separate patch, with comments, if that's OK with you.

> > @@ -67,24 +67,27 @@ void ftrace_record_recursion(unsigned long ip, unsigned long parent_ip)

[...]

> >  	old = cmpxchg(&recursed_functions[index].ip, 0, ip);
> >  	if (old != 0) {
> >  		/* Did something else already added this for us? */
> > -		if (old == ip)
> > +		if (old == ip) {
> > +			cached_function = ip;
> >  			return;
> > +		}
>
> I think we do not need the above. If old == ip, then whatever set it before
> should have updated the cached_function.

Makes sense, I'll drop it in v1.  (The cache only holds one entry, so
another ip could have replaced it in the meantime, but that just means a
later call takes the slow path.)

Thanks,
  Andrea

[1] https://lore.kernel.org/r/20260908073906.31781-1-yangtiezhu@loongson.cn