On 2024/9/10 3:02, Michal Koutný wrote:
On Thu, Sep 05, 2024 at 01:41:29PM GMT, Chen Ridong <chenridong@xxxxxxxxxx> wrote:
When a cgroup is frozen/unfrozen, it will always propagate down to up.
bottom up
However it is unnecessary to propagate to the top every time. This patch
aims to reduce redundant propagation for cgroup_propagate_frozen.
For example, subtree like:
a
|
b
/ | \
c d e
If c is frozen, and d and e are not frozen now, it doesn't have to
propagate to a; Only when c, d and e are all frozen, b and a could be set
to frozen. Therefore, if nr_frozen_descendants is not equal to
nr_descendants, just stop propagate. If a descendant is frozen, the
parent's nr_frozen_descendants add child->nr_descendants + 1. This can
reduce redundant propagation.
So, IIUC, this saves the updates by aggregating updates of
nr_frozen_descendants into chunks sized same like each child's
nr_descendants, otherwise there's no effect to propagate upward,
correct?
correct.
This would deserve some update of the
cgroup_freezer_state.nr_frozen_descendants comment.
Signed-off-by: Chen Ridong <chenridong@xxxxxxxxxx>
---
kernel/cgroup/freezer.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/kernel/cgroup/freezer.c b/kernel/cgroup/freezer.c
index 02af6c1fa957..e4bcc41b6a30 100644
--- a/kernel/cgroup/freezer.c
+++ b/kernel/cgroup/freezer.c
@@ -13,7 +13,7 @@
*/
static void cgroup_propagate_frozen(struct cgroup *cgrp, bool frozen)
{
- int desc = 1;
+ struct cgroup *child = cgrp;
/*
* If the new state is frozen, some freezing ancestor cgroups may change
@@ -23,23 +23,38 @@ static void cgroup_propagate_frozen(struct cgroup *cgrp, bool frozen)
*/
while ((cgrp = cgroup_parent(cgrp))) {
if (frozen) {
- cgrp->freezer.nr_frozen_descendants += desc;
+ /*
+ * A cgroup is frozen, parent nr frozen descendants should add
+ * nr cgroups of the entire subtree , including child itself.
+ */
+ cgrp->freezer.nr_frozen_descendants += child->nr_descendants + 1;
Shouldn't this be
cgrp->freezer.nr_frozen_descendants += child->nr_frozen_descendants + 1;
?
child->nr_frozen_descendants should be equal to child->nr_descendants if
the subtree is already frozen.
+
+ /*
+ * If there is other descendant is not frozen,
+ * cgrp and its parent couldn't be frozen, just break
+ */
+ if (cgrp->freezer.nr_frozen_descendants !=
+ cgrp->nr_descendants)
+ break;
Parent's (and ancestral) nr_frozen_descendants would be out of date.
This should be correct also wrt cgroup creation and removal.
Before calling cgroup_freeze, cgroup_freeze_write have hold the
cgroup_mutex, could parent's nr_frozen_descendants be changed?
+
+ child = cgrp;
This is same in both branches, so it can be taken outside (maybe even
replace while() with for() if it's cleaner.)
I will try.
if (!test_bit(CGRP_FROZEN, &cgrp->flags) &&
- test_bit(CGRP_FREEZE, &cgrp->flags) &&
- cgrp->freezer.nr_frozen_descendants ==
- cgrp->nr_descendants) {
+ test_bit(CGRP_FREEZE, &cgrp->flags)) {
set_bit(CGRP_FROZEN, &cgrp->flags);
cgroup_file_notify(&cgrp->events_file);
TRACE_CGROUP_PATH(notify_frozen, cgrp, 1);
- desc++;
}
} else {
- cgrp->freezer.nr_frozen_descendants -= desc;
+ cgrp->freezer.nr_frozen_descendants -= (child->nr_descendants + 1);
nit: here you add parentheses but not in the frozen branch
+
+ child = cgrp;
if (test_bit(CGRP_FROZEN, &cgrp->flags)) {
clear_bit(CGRP_FROZEN, &cgrp->flags);
cgroup_file_notify(&cgrp->events_file);
TRACE_CGROUP_PATH(notify_frozen, cgrp, 0);
- desc++;
+ } else {
+ /* If parent is unfrozen, don't have to propagate more */
+ break;
}
}
}
I understand the idea roughly. The code in cgroup_propagate_frozen was
(and stays after your change) slightly difficult to read smoothly but I
think if it can be changed, the reduced (tree) iteration may end up
correct.
Thanks,
Michal
Thanks, I will update in next patch.
Best regards,
Ridong
