Skip to contentSkip to conversation

cgroup: rstat: force flush on css exit

PATCH In review Shakeel Butt 2 participants 3 messages Dec 4, 2025 – Dec 15, 2025

Subsystems (2)
Memory management cgroup
lore
Share & tools
lore

Shakeel Butt

author
#0 lore raw

PATCH 1/1 cgroup: rstat: force flush on css exit

Cuurently the rstat update side is lockless and transfers the css of cgroup whose stats has been updated through lockless list (llist). There is an expected race where rstat updater skips adding css to the llist because it was already in the list but the flusher might not see those updates done by the skipped updater.

Usually the subsequent updater will take care of such situation but what if the skipped updater was the last updater before the cgroup is removed by the user. In that case stat updates by the skipped updater will be lost. To avoid that let's always flush the stats of the offlined cgroup.

Signed-off-by Shakeel Butt <[email protected]>
Fixes 36df6e3dbd7e ("cgroup: make css_rstat_updated nmi safe")
---
 kernel/cgroup/rstat.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
kernel/cgroup/rstat.c +10 -0
index a198e40c799b..91b34ebd5370 100644
@@ -283,6 +283,16 @@ static struct cgroup_subsys_state *css_rstat_updated_list(
 
 	css_process_update_tree(root->ss, cpu);
 
+	/*
+	 * We allow race between rstat updater and flusher which can cause a
+	 * scenario where the updater skips adding the css to the list but the
+	 * flusher might not see updater's updates. Usually the subsequent
+	 * updater would take care of that but what if that was the last updater
+	 * on that CPU before getting removed. Handle that scenario here.
+	 */
+	if (!css_is_online(root))
+		__css_process_update_tree(root, cpu);
+
 	/* Return NULL if this subtree is not on-list */
 	if (!rstatc->updated_next)
 		return NULL;
Signature · 1 line
2.47.3
maintainer
#1 lore raw

Hi Shakeel.

Quoted 5 lines · Shakeel Butt

Cuurently the rstat update side is lockless and transfers the css of cgroup whose stats has been updated through lockless list (llist). There is an expected race where rstat updater skips adding css to the llist because it was already in the list but the flusher might not see those updates done by the skipped updater.

Notice that there's css_rstat_flush() in
css_free_rwork_fn()/css_rstat_exit().

Quoted 4 lines

Usually the subsequent updater will take care of such situation but what if the skipped updater was the last updater before the cgroup is removed by the user. In that case stat updates by the skipped updater will be lost. To avoid that let's always flush the stats of the offlined cgroup.

Are you sure here that this is the different cause of the loss than the other with unlocked cmpxchg you posted later?

Quoted 4 lines
kernel/cgroup/rstat.c +0 -0
@@ -283,6 +283,16 @@ static struct cgroup_subsys_state *css_rstat_updated_list(
 
 	css_process_update_tree(root->ss, cpu);

kernel/cgroup/rstat.c static struct cgroup_subsys_state *css_rstat_updated_list( · line 295

Quoted 10 lines
kernel/cgroup/rstat.c +10 -0
+	/*
+	 * We allow race between rstat updater and flusher which can cause a
+	 * scenario where the updater skips adding the css to the list but the
+	 * flusher might not see updater's updates. Usually the subsequent
+	 * updater would take care of that but what if that was the last updater
+	 * on that CPU before getting removed. Handle that scenario here.
+	 */
+	if (!css_is_online(root))
+		__css_process_update_tree(root, cpu);
+

I'm thinking about this approach:

@@ -482,6 +484,15 @@ void css_rstat_exit(struct cgroup_subsys_state *css)

        if (!css->rstat_cpu)
                return;

+ /*

+        * We allow race between rstat updater and flusher which can cause a
+        * scenario where the updater skips adding the css to the list but the
+        * flusher might not see updater's updates. Usually the subsequent
+        * updater would take care of that but what if that was the last updater
+        * on that CPU before getting removed. Handle that scenario here.
+        */
+       for_each_possible_cpu(cpu)
+               css_rstat_updated(css, cpu);
        css_rstat_flush(css);

        /* sanity check */

because that moves the special treating from relatively commonn css_rstat_updated_list() to only cgroup_exit().

(I didn't check this wouldn't break anything.)

Thanks,
Michal

author
#2 lore raw
Quoted 12 lines · Michal Koutný

Hi Shakeel.

Quoted 5 lines · Shakeel Butt

Cuurently the rstat update side is lockless and transfers the css of cgroup whose stats has been updated through lockless list (llist). There is an expected race where rstat updater skips adding css to the llist because it was already in the list but the flusher might not see those updates done by the skipped updater.

Notice that there's css_rstat_flush() in
css_free_rwork_fn()/css_rstat_exit().

Quoted 8 lines
Quoted 4 lines

Usually the subsequent updater will take care of such situation but what if the skipped updater was the last updater before the cgroup is removed by the user. In that case stat updates by the skipped updater will be lost. To avoid that let's always flush the stats of the offlined cgroup.

Are you sure here that this is the different cause of the loss than the other with unlocked cmpxchg you posted later?

I didn't see any stats loss due to this specific case but I found this on code inspection while debugging the other issue.

Quoted 34 lines
Quoted 4 lines
kernel/cgroup/rstat.c +0 -0
@@ -283,6 +283,16 @@ static struct cgroup_subsys_state *css_rstat_updated_list(
 
 	css_process_update_tree(root->ss, cpu);

kernel/cgroup/rstat.c static struct cgroup_subsys_state *css_rstat_updated_list( · line 295

Quoted 10 lines
kernel/cgroup/rstat.c +10 -0
+	/*
+	 * We allow race between rstat updater and flusher which can cause a
+	 * scenario where the updater skips adding the css to the list but the
+	 * flusher might not see updater's updates. Usually the subsequent
+	 * updater would take care of that but what if that was the last updater
+	 * on that CPU before getting removed. Handle that scenario here.
+	 */
+	if (!css_is_online(root))
+		__css_process_update_tree(root, cpu);
+

I'm thinking about this approach:

kernel/cgroup/rstat.c +9 -0
@@ -482,6 +484,15 @@ void css_rstat_exit(struct cgroup_subsys_state *css)
        if (!css->rstat_cpu)
                return;
 
+       /*
+        * We allow race between rstat updater and flusher which can cause a
+        * scenario where the updater skips adding the css to the list but the
+        * flusher might not see updater's updates. Usually the subsequent
+        * updater would take care of that but what if that was the last updater
+        * on that CPU before getting removed. Handle that scenario here.
+        */
+       for_each_possible_cpu(cpu)
+               css_rstat_updated(css, cpu);
        css_rstat_flush(css);
 
        /* sanity check */
Quoted 4 lines

because that moves the special treating from relatively commonn css_rstat_updated_list() to only cgroup_exit().

(I didn't check this wouldn't break anything.)

Yes I think this is much better. We just need to disable preemption for the assert within css_rstat_updated().