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/[email protected]
