Re: [PATCH 2/2] cgroup: explain the race between updater and flusher

[Shakeel Butt <shakeel.butt@linux.dev>]

On Thu, Jul 03, 2025 at 03:29:16PM -0700, Paul E. McKenney wrote:
> On Thu, Jul 03, 2025 at 01:00:12PM -0700, Shakeel Butt wrote:
> > Currently the rstat updater and the flusher can race and cause a
> > scenario where the stats updater skips adding the css to the lockless
> > list but the flusher might not see those updates done by the skipped
> > updater. This is benign race and the subsequent flusher will flush those
> > stats and at the moment there aren't any rstat users which are not fine
> > with this kind of race. However some future user might want more
> > stricter guarantee, so let's add appropriate comments and data_race()
> > tags to ease the job of future users.
> > 
> > Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> > ---
> >  kernel/cgroup/rstat.c | 32 +++++++++++++++++++++++++++++---
> >  1 file changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
> > index c8a48cf83878..b98c03b1af25 100644
> > --- a/kernel/cgroup/rstat.c
> > +++ b/kernel/cgroup/rstat.c
> > @@ -60,6 +60,12 @@ static inline struct llist_head *ss_lhead_cpu(struct cgroup_subsys *ss, int cpu)
> >   * Atomically inserts the css in the ss's llist for the given cpu. This is
> >   * reentrant safe i.e. safe against softirq, hardirq and nmi. The ss's llist
> >   * will be processed at the flush time to create the update tree.
> > + *
> > + * NOTE: if the user needs the guarantee that the updater either add itself in
> > + * the lockless list or the concurrent flusher flushes its updated stats, a
> > + * memory barrier is needed before the call to css_rstat_updated() i.e. a
> > + * barrier after updating the per-cpu stats and before calling
> > + * css_rstat_updated().
> >   */
> >  __bpf_kfunc void css_rstat_updated(struct cgroup_subsys_state *css, int cpu)
> >  {
> > @@ -86,8 +92,13 @@ __bpf_kfunc void css_rstat_updated(struct cgroup_subsys_state *css, int cpu)
> >  		return;
> >  
> >  	rstatc = css_rstat_cpu(css, cpu);
> > -	/* If already on list return. */
> > -	if (llist_on_list(&rstatc->lnode))
> > +	/*
> > +	 * If already on list return. This check is racy and smp_mb() is needed
> > +	 * to pair it with the smp_mb() in css_process_update_tree() if the
> > +	 * guarantee that the updated stats are visible to concurrent flusher is
> > +	 * needed.
> > +	 */
> > +	if (data_race(llist_on_list(&rstatc->lnode)))
> 
> OK, I will bite...
> 
> Why is this needed given the READ_ONCE() that the earlier patch added to
> llist_on_list()?
> 
> >  		return;
> >  
> >  	/*
> > @@ -145,9 +156,24 @@ static void css_process_update_tree(struct cgroup_subsys *ss, int cpu)
> >  	struct llist_head *lhead = ss_lhead_cpu(ss, cpu);
> >  	struct llist_node *lnode;
> >  
> > -	while ((lnode = llist_del_first_init(lhead))) {
> > +	while ((lnode = data_race(llist_del_first_init(lhead)))) {
> 
> And for this one, why not make init_llist_node(), which is invoked from
> llist_del_first_init(), do a WRITE_ONCE()?
> 

Let me answer this one first. The previous patch actually made
init_llist_node() do WRITE_ONCE().

So the actual question is why do we need
data_race([READ|WRITE]_ONCE()) instead of just [READ|WRITE]_ONCE()?
Actually I had the similar question myself and found the following
comment in include/linux/compiler.h:

/**
 * data_race - mark an expression as containing intentional data races
 *
 * This data_race() macro is useful for situations in which data races
 * should be forgiven.  One example is diagnostic code that accesses
 * shared variables but is not a part of the core synchronization design.
 * For example, if accesses to a given variable are protected by a lock,
 * except for diagnostic code, then the accesses under the lock should
 * be plain C-language accesses and those in the diagnostic code should
 * use data_race().  This way, KCSAN will complain if buggy lockless
 * accesses to that variable are introduced, even if the buggy accesses
 * are protected by READ_ONCE() or WRITE_ONCE().
 *
 * This macro *does not* affect normal code generation, but is a hint
 * to tooling that data races here are to be ignored.  If the access must
 * be atomic *and* KCSAN should ignore the access, use both data_race()
 * and READ_ONCE(), for example, data_race(READ_ONCE(x)).
 */

IIUC correctly, I need to protect llist_node against tearing and as well
as tell KCSAN to ignore the access for race then I should use both.
Though I think KCSAN treat [READ|WRITE]_ONCE similar to data_race(), so
it kind of seem redundant but I think at least I want to convey that we
need protection against tearing and ignore KCSAN and using both conveys
that. Let me know if you think otherwise.

thanks a lot for taking a look.