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.)
> }
>
> - if (cgroup_update_frozen_flag(cgrp, frozen))
> - desc++;
> + /* No change, stop propagate */
> + if (!cgroup_update_frozen_flag(cgrp, frozen))
> + break;
> +
> + parent = cgroup_parent(cgrp);
> + parent->freezer.nr_frozen_descendants += deta;
Thanks,
Michal
Attachment:
signature.asc
Description: PGP signature
