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@xxxxxxxxx>
> > ---
> > 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.
