From: Chen Ridong <chenridong@huawei.com>
Both online_cnt and nr_descendants can indicate whether acitive
descendants exist. To make code simple, remove redundancy online_cnt,
use nr_descendants instead.
Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
include/linux/cgroup-defs.h | 6 ------
kernel/cgroup/cgroup.c | 13 ++++---------
kernel/cgroup/debug.c | 2 +-
3 files changed, 5 insertions(+), 16 deletions(-)
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 6b93a64115fe..d084c5c34c09 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -210,12 +210,6 @@ struct cgroup_subsys_state {
*/
u64 serial_nr;
- /*
- * Incremented by online self and children. Used to guarantee that
- * parents are not offlined before their children.
- */
- atomic_t online_cnt;
-
/* percpu_ref killing and RCU release */
struct work_struct destroy_work;
struct rcu_work destroy_rwork;
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 79b1d79f86a3..5eb747a038f7 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5607,7 +5607,6 @@ static void init_and_link_css(struct cgroup_subsys_state *css,
INIT_LIST_HEAD(&css->sibling);
INIT_LIST_HEAD(&css->children);
css->serial_nr = css_serial_nr_next++;
- atomic_set(&css->online_cnt, 0);
if (cgroup_parent(cgrp)) {
css->parent = cgroup_css(cgroup_parent(cgrp), ss);
@@ -5631,12 +5630,8 @@ static int online_css(struct cgroup_subsys_state *css)
css->flags |= CSS_ONLINE;
rcu_assign_pointer(css->cgroup->subsys[ss->id], css);
- atomic_inc(&css->online_cnt);
- if (css->parent) {
- atomic_inc(&css->parent->online_cnt);
- while ((css = css->parent))
- css->nr_descendants++;
- }
+ while ((css = css->parent))
+ css->nr_descendants++;
}
return ret;
}
@@ -5949,7 +5944,7 @@ static void css_killed_work_fn(struct work_struct *work)
css_put(css);
/* @css can't go away while we're holding cgroup_mutex */
css = css->parent;
- } while (css && atomic_dec_and_test(&css->online_cnt));
+ } while (css && css_is_dying(css) && !css->nr_descendants);
cgroup_unlock();
}
@@ -5960,7 +5955,7 @@ static void css_killed_ref_fn(struct percpu_ref *ref)
struct cgroup_subsys_state *css =
container_of(ref, struct cgroup_subsys_state, refcnt);
- if (atomic_dec_and_test(&css->online_cnt)) {
+ if (!css->nr_descendants) {
INIT_WORK(&css->destroy_work, css_killed_work_fn);
queue_work(cgroup_offline_wq, &css->destroy_work);
}
diff --git a/kernel/cgroup/debug.c b/kernel/cgroup/debug.c
index 80aa3f027ac3..fc2f3216173a 100644
--- a/kernel/cgroup/debug.c
+++ b/kernel/cgroup/debug.c
@@ -226,7 +226,7 @@ static int cgroup_subsys_states_read(struct seq_file *seq, void *v)
css->parent->id);
seq_printf(seq, "%2d: %-4s\t- %p[%d] %d%s\n", ss->id, ss->name,
css, css->id,
- atomic_read(&css->online_cnt), pbuf);
+ css->nr_descendants, pbuf);
}
cgroup_kn_unlock(of->kn);
--
2.34.1
On 2025/8/26 11:40, Chen Ridong wrote:
> @@ -5949,7 +5944,7 @@ static void css_killed_work_fn(struct work_struct *work)
> css_put(css);
> /* @css can't go away while we're holding cgroup_mutex */
> css = css->parent;
> - } while (css && atomic_dec_and_test(&css->online_cnt));
> + } while (css && css_is_dying(css) && !css->nr_descendants);
>
> cgroup_unlock();
> }
> @@ -5960,7 +5955,7 @@ static void css_killed_ref_fn(struct percpu_ref *ref)
> struct cgroup_subsys_state *css =
> container_of(ref, struct cgroup_subsys_state, refcnt);
>
> - if (atomic_dec_and_test(&css->online_cnt)) {
> + if (!css->nr_descendants) {
> INIT_WORK(&css->destroy_work, css_killed_work_fn);
> queue_work(cgroup_offline_wq, &css->destroy_work);
> }
Hi Michal,
Thank you point out the data race issue, Can I modify the code just like:
@@ -5944,12 +5939,13 @@ static void css_killed_work_fn(struct work_struct *work)
cgroup_lock();
- do {
+ /* The CSS can only be taken offline when it has no living descendants. */
+ while (css && css_is_dying(css) && !css->nr_descendants) {
offline_css(css);
css_put(css);
/* @css can't go away while we're holding cgroup_mutex */
css = css->parent;
- } while (css && atomic_dec_and_test(&css->online_cnt));
+ }
cgroup_unlock();
}
@@ -5960,10 +5956,9 @@ static void css_killed_ref_fn(struct percpu_ref *ref)
struct cgroup_subsys_state *css =
container_of(ref, struct cgroup_subsys_state, refcnt);
- if (atomic_dec_and_test(&css->online_cnt)) {
- INIT_WORK(&css->destroy_work, css_killed_work_fn);
- queue_work(cgroup_offline_wq, &css->destroy_work);
- }
+ INIT_WORK(&css->destroy_work, css_killed_work_fn);
+ queue_work(cgroup_offline_wq, &css->destroy_work);
+
}
--
Best regards,
Ridong
Hello Ridong.
On Tue, Aug 26, 2025 at 03:40:20AM +0000, Chen Ridong <chenridong@huaweicloud.com> wrote:
> @@ -5949,7 +5944,7 @@ static void css_killed_work_fn(struct work_struct *work)
> css_put(css);
> /* @css can't go away while we're holding cgroup_mutex */
> css = css->parent;
> - } while (css && atomic_dec_and_test(&css->online_cnt));
> + } while (css && css_is_dying(css) && !css->nr_descendants);
Here it's OK...
>
> cgroup_unlock();
> }
> @@ -5960,7 +5955,7 @@ static void css_killed_ref_fn(struct percpu_ref *ref)
> struct cgroup_subsys_state *css =
> container_of(ref, struct cgroup_subsys_state, refcnt);
>
> - if (atomic_dec_and_test(&css->online_cnt)) {
> + if (!css->nr_descendants) {
> INIT_WORK(&css->destroy_work, css_killed_work_fn);
> queue_work(cgroup_offline_wq, &css->destroy_work);
> }
... but here in percpu_ref's confirm callback you're accessing
nr_descendants without cgroup_mutex where the atomic would have
prevented the data race.
Also the semantics of online_cnt and nr_descendants is slightly
different -- killed vs offlined. Or can you add a description why
they're same (after workqueue split)?
Thanks,
Michal
On 2025/8/26 22:14, Michal Koutný wrote:
> Hello Ridong.
>
> On Tue, Aug 26, 2025 at 03:40:20AM +0000, Chen Ridong <chenridong@huaweicloud.com> wrote:
>> @@ -5949,7 +5944,7 @@ static void css_killed_work_fn(struct work_struct *work)
>> css_put(css);
>> /* @css can't go away while we're holding cgroup_mutex */
>> css = css->parent;
>> - } while (css && atomic_dec_and_test(&css->online_cnt));
>> + } while (css && css_is_dying(css) && !css->nr_descendants);
>
> Here it's OK...
>
>>
>> cgroup_unlock();
>> }
>> @@ -5960,7 +5955,7 @@ static void css_killed_ref_fn(struct percpu_ref *ref)
>> struct cgroup_subsys_state *css =
>> container_of(ref, struct cgroup_subsys_state, refcnt);
>>
>> - if (atomic_dec_and_test(&css->online_cnt)) {
>> + if (!css->nr_descendants) {
>> INIT_WORK(&css->destroy_work, css_killed_work_fn);
>> queue_work(cgroup_offline_wq, &css->destroy_work);
>> }
>
> ... but here in percpu_ref's confirm callback you're accessing
> nr_descendants without cgroup_mutex where the atomic would have
> prevented the data race.
>
Thank you very much, Michal, I miss this case.
> Also the semantics of online_cnt and nr_descendants is slightly
> different -- killed vs offlined. Or can you add a description why
> they're same (after workqueue split)?
>
The nr_descendants value does not include the dying CSS; it only reflects the number of currently
living descendants. Moreover, a CSS can only be taken offline when no living CSS remains. Therefore,
I believe the online_cnt is no longer necessary. This is unrelated to workqueue splitting.
--
Best regards,
Ridong
© 2016 - 2025 Red Hat, Inc.