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?
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;
?
> +
> + /*
> + * 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.
> +
> + child = cgrp;
This is same in both branches, so it can be taken outside (maybe even
replace while() with for() if it's cleaner.)
> 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
Attachment:
signature.asc
Description: PGP signature
