Re: [PATCH v1 -next 2/3] cgroup/freezer: Reduce redundant propagation for cgroup_propagate_frozen

[Michal Koutný]

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