On Fri, Jul 24, 2026 at 02:20:48PM +0100, David Laight wrote:
> On Thu, 23 Jul 2026 18:04:30 +0800
> Zqiang <[email protected]> wrote:
>
> > When CONFIG_KASAN=y and CONFIG_KASAN_STACK=y builds, the
> > show_rcu_gp_kthreads() exceeds the 1024-byte frame-size limit:
> >
> ...
> > rcu_for_each_node_breadth_first(rnp) {
> > if (ULONG_CMP_GE(READ_ONCE(rcu_state.gp_seq),
> > READ_ONCE(rnp->gp_seq_needed)) &&
> > !data_race(READ_ONCE(rnp->qsmask)) &&
> > !data_race(READ_ONCE(rnp->boost_tasks)) &&
> > !data_race(READ_ONCE(rnp->exp_tasks)) &&
> > !data_race(READ_ONCE(rnp->gp_tasks)))
> > continue;
> > - pr_info("\trcu_node %d:%d ->gp_seq %ld ->gp_seq_needed %ld
> > ->qsmask %#lx %c%c%c%c ->n_boosts %ld\n",
> > - rnp->grplo, rnp->grphi,
> > - (long)data_race(READ_ONCE(rnp->gp_seq)),
> > - (long)data_race(READ_ONCE(rnp->gp_seq_needed)),
> > - data_race(READ_ONCE(rnp->qsmask)),
> > - ".b"[!!data_race(READ_ONCE(rnp->boost_kthread_task))],
> > - ".B"[!!data_race(READ_ONCE(rnp->boost_tasks))],
> > - ".E"[!!data_race(READ_ONCE(rnp->exp_tasks))],
> > - ".G"[!!data_race(READ_ONCE(rnp->gp_tasks))],
> > - data_race(READ_ONCE(rnp->n_boosts)));
>
> Isn't that code carefully reading all the values twice?
You are quite right, it does, and it did before Zqiang's patch. I would
welcome a patch that carefully used locals to avoid the double reads,
as that could avoid some debug confusion that might otherwise result
due to low-probability races with updates to these same variables.
However, this function is invoked quite rarely in error conditions,
so I would also avoid holding anything up waiting for such a patch.
> Also the "ab"[!!val] generates far worse code than the more obvious (val ?
> 'a' : 'b').
> For the former gcc indexes a constant string, the latter is done using
> arithmetic.
This code is invoked quite rarely, so I am not concerned about the
overhead and I like the compactness. In the longer term, perhaps
compilers will recognize the pattern and optimize it. Or not, who
knows? ;-)
> I'm sure there is a good reason for data_race(READ_ONCE(xxx)) ...
And there is!!!
The data_race() says "ignore the contents from a data-race viewpoint"
and the READ_ONCE() says "carefully avoid destructive optimizations that
might otherwise be applied to this load."
For an example, suppose that we had a global variable x that was accessed
only under a lock. Except that we have lockless access for debugging
output like that in Zqiang's patch. We want the normal lock-protected
accesses to be plain C-language accesses so that any accidental (and thus
likely buggy) lockless accesses are spotted by KCSAN, but we very much
to *not* want to acquire the lock when doing the debug output, because
if the lock was held, we might get a debug-output-free hang just when
we are in the greatest need of that debugging output.
So we use normal C-language accesses for the non-debug uses, and we use
data_race(READ_ONCE(x)) for the debugging output.
Does that make sense?
Thanx, Paul
