On 2024/9/26 1:46, Michal Koutný wrote:
On Sun, Sep 15, 2024 at 07:13:07AM GMT, Chen Ridong <chenridong@xxxxxxxxxx> wrote:
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index dd1ecab99eeb..41e4e5a7ae55 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -401,7 +401,9 @@ struct cgroup_freezer_state {
/* Fields below are protected by css_set_lock */
- /* Number of frozen descendant cgroups */
+ /* Aggregating frozen descendant cgroups, only when all
+ * descendants of a child are frozen will the count increase.
+ */
int nr_frozen_descendants;
/*
diff --git a/kernel/cgroup/freezer.c b/kernel/cgroup/freezer.c
index bf1690a167dd..4ee33198d6fb 100644
--- a/kernel/cgroup/freezer.c
+++ b/kernel/cgroup/freezer.c
@@ -35,27 +35,34 @@ static bool cgroup_update_frozen_flag(struct cgroup *cgrp, bool frozen)
*/
static void cgroup_propagate_frozen(struct cgroup *cgrp, bool frozen)
{
- int desc = 1;
-
+ int deta;
delta
+ struct cgroup *parent;
I'd suggest here something like
/* root cgroup never changes freeze state */
if (WARN_ON(cgroup_parent(cgrp))
return;
so that the parent-> dereference below is explicitly safe.
/*
* If the new state is frozen, some freezing ancestor cgroups may change
* their state too, depending on if all their descendants are frozen.
*
* Otherwise, all ancestor cgroups are forced into the non-frozen state.
*/
- while ((cgrp = cgroup_parent(cgrp))) {
+ for (; cgrp; cgrp = cgroup_parent(cgrp)) {
if (frozen) {
- cgrp->freezer.nr_frozen_descendants += desc;
+ /* If freezer is not set, or cgrp has descendants
+ * that are not frozen, cgrp can't be frozen
+ */
if (!test_bit(CGRP_FREEZE, &cgrp->flags) ||
(cgrp->freezer.nr_frozen_descendants !=
- cgrp->nr_descendants))
- continue;
+ cgrp->nr_descendants))
+ break;
+ deta = cgrp->freezer.nr_frozen_descendants + 1;
} else {
- cgrp->freezer.nr_frozen_descendants -= desc;
+ deta = -(cgrp->freezer.nr_frozen_descendants + 1);
In this branch, if cgrp is unfrozen, delta = -1 is cgrp itself,
however is delta = -cgrp->freezer.nr_frozen_descendants warranted?
What if they are frozen empty children (of cgrp)? They likely shouldn't
be subtracted from ancestors nf_frozen_descendants.
(This refers to a situation when
C CGRP_FREEZE is set
|\
D E both CGRP_FREEZE is set
and an unfrozen task is migrated into C which would make C (temporarily)
unfrozen but not D nor E.)
Thank you, Michal.
I sorry I missed this situation.
If unfreezing a cgroup, it seems it has to propagate to the top.
After consideration, I modify this function.
the following is acceptable?
/*
* Propagate the cgroup frozen state upwards by the cgroup tree.
*/
static void cgroup_propagate_frozen(struct cgroup *cgrp, bool frozen)
{
int deta = 0;
struct cgroup *parent;
/*
* case1: If the new state is frozen, some freezing ancestor cgroups
may change
* their state too, depending on if all their descendants are frozen.
*
* case2: unfrozen, all ancestor cgroups are forced into the non-frozen
state.
*/
for (; cgrp; cgrp = cgroup_parent(cgrp)) {
if (frozen) {
/* If freezer is not set, or cgrp has descendants
* that are not frozen, cgrp can't be frozen
*/
if (!test_bit(CGRP_FREEZE, &cgrp->flags) ||
(cgrp->freezer.nr_frozen_descendants !=
cgrp->nr_descendants))
break;
/* No change, stop propagate */
if (!cgroup_update_frozen_flag(cgrp, frozen))
break;
deta = cgrp->freezer.nr_frozen_descendants + 1;
} else {
/* case2: have to propagate all ancestor */
if (cgroup_update_frozen_flag(cgrp, frozen))
deta++;
}
parent = cgroup_parent(cgrp);
parent->freezer.nr_frozen_descendants += deta;
}
}
Best regards,
Ridong
