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

[Chen Ridong]

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