[PATCH v1 -next 0/3] Some optimizations about freezer

[PATCH v1 -next 0/3] Some optimizations about freezer

[Chen Ridong]

We optimized the freezer to reduce redundant loops. We add a selftest to
ensure our optimizations cause no harm, and we confirmed that the
performance can be improved.

We tested the following subtree: D, E, F and G each have n children.
        A
       / \
      B   C
    / | \  \
    D E F   G
   /  |  \   \
 1-n 1-n 1-n  0-n

Our test is to freeze A B C D E F G, and then unfreeze A B C D E F G.
We measured the elapsed time.

	BEFORE(ns)	AFTER(ns)	SAVED(ns)
n=10	142679950	139666014	3,013,936
n=100	199832160	192773032	7,059,128
n=1000	488595100	414901570	73,693,530

As shown above, the larger the value of n, the more significant the time
savings can be. If tested with a deeper hierarchy, the difference could
become even more apparent.

Chen Ridong (3):
  cgroup/freezer: Reduce redundant traversal for cgroup_freeze
  cgroup/freezer: Reduce redundant propagation for
    cgroup_propagate_frozen
  cgroup/freezer: Add freeze selftest

 include/linux/cgroup-defs.h                   |   2 +-
 kernel/cgroup/freezer.c                       |  72 ++++++++----
 .../testing/selftests/cgroup/test_freezer.sh  | 111 ++++++++++++++++++
 3 files changed, 160 insertions(+), 25 deletions(-)
 create mode 100755 tools/testing/selftests/cgroup/test_freezer.sh

-- 
2.34.1

[PATCH v1 -next 1/3] cgroup/freezer: Reduce redundant traversal for cgroup_freeze

[Chen Ridong]

Whether a cgroup is frozen is determined solely by whether it is set to
to be frozen and whether its parent is frozen. Currently, when is cgroup
is frozen or unfrozen, it iterates through the entire subtree to freeze
or unfreeze its descentdants. However, this is unesessary for a cgroup
that does not change its effective frozen status. This path aims to skip
the subtree if its parent does not have a change in effective freeze.

For an example, subtree like, a-b-c-d-e-f-g, when a is frozen, the
entire tree is frozen. If we freeze b and c again, it is unesessary to
iterate d, e, f and g. So does that If we unfreeze b/c.

Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
 include/linux/cgroup-defs.h |  2 +-
 kernel/cgroup/freezer.c     | 41 ++++++++++++++++++++++---------------
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index d3aca2c61154..e9d175a1bf94 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -393,7 +393,7 @@ struct cgroup_freezer_state {
 	bool freeze;
 
 	/* Should the cgroup actually be frozen? */
-	int e_freeze;
+	bool e_freeze;
 
 	/* Fields below are protected by css_set_lock */
 
diff --git a/kernel/cgroup/freezer.c b/kernel/cgroup/freezer.c
index 617861a54793..02af6c1fa957 100644
--- a/kernel/cgroup/freezer.c
+++ b/kernel/cgroup/freezer.c
@@ -45,6 +45,21 @@ static void cgroup_propagate_frozen(struct cgroup *cgrp, bool frozen)
 	}
 }
 
+/*
+ * Update cgroup freezer.e_freeze
+ * e_freeze will be set to true if freeze == true or parent's e_freeze == true
+ */
+static inline void cgroup_update_efreeze(struct cgroup *cgrp)
+{
+	struct cgroup *parent = cgroup_parent(cgrp);
+	bool p_e = false;
+
+	if (parent)
+		p_e = parent->freezer.e_freeze;
+
+	cgrp->freezer.e_freeze = cgrp->freezer.freeze | p_e;
+}
+
 /*
  * Revisit the cgroup frozen state.
  * Checks if the cgroup is really frozen and perform all state transitions.
@@ -262,6 +277,7 @@ void cgroup_freeze(struct cgroup *cgrp, bool freeze)
 	struct cgroup_subsys_state *css;
 	struct cgroup *dsct;
 	bool applied = false;
+	bool old_e;
 
 	lockdep_assert_held(&cgroup_mutex);
 
@@ -282,22 +298,15 @@ void cgroup_freeze(struct cgroup *cgrp, bool freeze)
 		if (cgroup_is_dead(dsct))
 			continue;
 
-		if (freeze) {
-			dsct->freezer.e_freeze++;
-			/*
-			 * Already frozen because of ancestor's settings?
-			 */
-			if (dsct->freezer.e_freeze > 1)
-				continue;
-		} else {
-			dsct->freezer.e_freeze--;
-			/*
-			 * Still frozen because of ancestor's settings?
-			 */
-			if (dsct->freezer.e_freeze > 0)
-				continue;
-
-			WARN_ON_ONCE(dsct->freezer.e_freeze < 0);
+		/*
+		 * If old e_freeze eq new e_freeze, no change, its children
+		 * will not be affected. So do nothing and skip the subtree
+		 */
+		old_e = dsct->freezer.e_freeze;
+		cgroup_update_efreeze(dsct);
+		if (dsct->freezer.e_freeze == old_e) {
+			css = css_rightmost_descendant(css);
+			continue;
 		}
 
 		/*
-- 
2.34.1

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

[Chen Ridong]

When a cgroup is frozen/unfrozen, it will always propagate down to 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.

Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
 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;
+
+			/*
+			 * 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;
+
+			child = cgrp;
 			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);
+
+			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;
 			}
 		}
 	}
-- 
2.34.1

[PATCH v1 -next 3/3] cgroup/freezer: Add freeze selftest

[Chen Ridong]

Add selftest to test cgroup.freeze and check cgroup.events state.

Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
 .../testing/selftests/cgroup/test_freezer.sh  | 111 ++++++++++++++++++
 1 file changed, 111 insertions(+)
 create mode 100755 tools/testing/selftests/cgroup/test_freezer.sh

diff --git a/tools/testing/selftests/cgroup/test_freezer.sh b/tools/testing/selftests/cgroup/test_freezer.sh
new file mode 100755
index 000000000000..7178980b1db8
--- /dev/null
+++ b/tools/testing/selftests/cgroup/test_freezer.sh
@@ -0,0 +1,111 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Test freeze hierarchy state
+#
+ROOT_PATH=/sys/fs/cgroup
+skip_test() {
+	echo "$1"
+	echo "Test SKIPPED"
+	exit 4 # ksft_skip
+}
+
+write_freeze() {
+	path=$1
+	value=$2
+	echo $value > $ROOT_PATH/$path/cgroup.freeze
+}
+
+get_event_frozen()
+{
+	path=$1
+	return $(cat $ROOT_PATH/$path/cgroup.events | grep frozen | awk '{print $2}')
+}
+
+# the test subtree:
+#        A
+#       / \
+#      B   C
+#    / | \  \
+#    D E F   G
+#   /
+#  H
+
+PATH_MATRIX=(
+	'A'
+	'A/B'
+	'A/C'
+	'A/B/D'
+	'A/B/E'
+	'A/B/F'
+	'A/C/G'
+	'A/B/D/H'
+)
+CGRP_CNT=${#PATH_MATRIX[@]}
+
+# Interface value to set/check
+ITF_MATRIX=(
+	#value write to freeze         expect event value
+	#A  B  C  D  E  F  G  H  |  A  B  C  D  E  F  G  H
+	'1  0  0  0  0  0  0  0     1  1  1  1  1  1  1  1'
+	'1  1  0  0  0  0  0  0     1  1  1  1  1  1  1  1'
+	'1  0  1  0  0  0  0  0     1  1  1  1  1  1  1  1'
+	'0  0  0  0  0  0  0  0     0  0  0  0  0  0  0  0'
+
+	'1  1  1  1  0  0  0  0     1  1  1  1  1  1  1  1'
+	'1  1  1  0  0  0  0  0     1  1  1  1  1  1  1  1'
+	'1  1  0  0  0  0  0  0     1  1  1  1  1  1  1  1'
+	'1  0  0  0  0  0  0  0     1  1  1  1  1  1  1  1'
+
+	'0  0  0  1  0  0  0  0     0  0  0  1  0  0  0  1'
+	'0  0  0  1  0  0  0  1     0  0  0  1  0  0  0  1'
+	'0  0  0  1  0  0  0  0     0  0  0  1  0  0  0  1'
+	'0  0  1  1  0  0  0  0     0  0  1  1  0  0  1  1'
+	'0  0  0  0  0  0  0  0     0  0  0  0  0  0  0  0'
+)
+
+prepare()
+{
+	for i in "${PATH_MATRIX[@]}" ; do
+		path="$ROOT_PATH/$i"
+		mkdir $path
+	done
+}
+
+do_clean()
+{
+	for(( i=$CGRP_CNT-1; i>=0; i-- ));do
+		path="/sys/fs/cgroup/${PATH_MATRIX[i]}"
+		rmdir $path
+	done
+}
+
+run_test()
+{
+	prepare
+	cgrp_cnt="${PATH_MATRIX[@]}"
+	for i in "${ITF_MATRIX[@]}" ; do
+		args=($i)
+		#write freeze
+		for(( j=0; j<$CGRP_CNT; j++ ));do
+			write_freeze "${PATH_MATRIX[j]}" ${args[j]}
+		done
+		#event frozen should eq expected value
+		for(( j=0; j<$CGRP_CNT; j++ ));do
+			get_event_frozen "${PATH_MATRIX[j]}"
+			if [ $? -ne ${args[j + $CGRP_CNT]} ];then
+				echo "failed: $i"
+				do_clean
+				exit 1
+			fi
+		done
+	done
+	do_clean
+}
+
+start_time=$(date +%s%N)
+run_test
+end_time=$(date +%s%N)
+elapsed_time=$((end_time - start_time))
+echo "Test PASSED, elapsed_time:$elapsed_time (ns)"
+exit 0
-- 
2.34.1

Re: [PATCH v1 -next 1/3] cgroup/freezer: Reduce redundant traversal for cgroup_freeze

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

On Thu, Sep 05, 2024 at 01:41:28PM GMT, Chen Ridong <chenridong@huawei.com> wrote:
> Whether a cgroup is frozen is determined solely by whether it is set to
> to be frozen and whether its parent is frozen. Currently, when is cgroup
> is frozen or unfrozen, it iterates through the entire subtree to freeze
> or unfreeze its descentdants. 

It's more to maintain the numeric freeze "layers".

> However, this is unesessary for a cgroup that does not change its
> effective frozen status.

True.

> +static inline void cgroup_update_efreeze(struct cgroup *cgrp)
> +{
> +	struct cgroup *parent = cgroup_parent(cgrp);
> +	bool p_e = false;
> +
> +	if (parent)
> +		p_e = parent->freezer.e_freeze;
> +
> +	cgrp->freezer.e_freeze = cgrp->freezer.freeze | p_e;

Better be || on bools

I'd open code this inside the loop of cgroup_freeze since it is not
context-less function and it relies on top-down processing.

Root cgrp cannot be frozen. You can bail out early in the beginning of
cgroup_freeze() (possibly with a WARN_ON) and then assume parent is
always valid when iterating.

I think maintaining the e_freeze in this "saturated arithmetic" form is
a sensible change.

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 4086 bytes --]

On Thu, Sep 05, 2024 at 01:41:29PM GMT, Chen Ridong <chenridong@huawei.com> 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@huawei.com>
> ---
>  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 -next 3/3] cgroup/freezer: Add freeze selftest

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 486 bytes --]

On Thu, Sep 05, 2024 at 01:41:30PM GMT, Chen Ridong <chenridong@huawei.com> wrote:
> Add selftest to test cgroup.freeze and check cgroup.events state.

There is already tools/testing/selftests/cgroup/test_freezer.c
Would you consider adding this as a new test for hierarchies there?
So that all freezer tests can be run at once and maintained in same style?

(There are already some creation/removal/event helpers so it shouldn't
be more difficult than the bash code.)

Thanks,
Michal


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 -next 0/3] Some optimizations about freezer

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

Hello.

On Thu, Sep 05, 2024 at 01:41:27PM GMT, Chen Ridong <chenridong@huawei.com> wrote:
> We optimized the freezer to reduce redundant loops. We add a selftest to
> ensure our optimizations cause no harm, and we confirmed that the
> performance can be improved.

I think these patches have potential to make the code also cleaner and
better to understand (the numbers from optimization don't impress me
that much at the moment).

I post more comments on the individual patches.


> We tested the following subtree: D, E, F and G each have n children.
>         A
>        / \
>       B   C
>     / | \  \
>     D E F   G
>    /  |  \   \
>  1-n 1-n 1-n  0-n

(Why is the last one different?)

> Our test is to freeze A B C D E F G, and then unfreeze A B C D E F G.

I would say measuring freezing/unfreezing of only A would give better
idea about the impact of the change.
(Going through all descendants manually would unfairly show greater
improvement.)

> We measured the elapsed time.
> 
> 	BEFORE(ns)	AFTER(ns)	SAVED(ns)
> n=10	142679950	139666014	3,013,936
> n=100	199832160	192773032	7,059,128
> n=1000	488595100	414901570	73,693,530

How many tasks were there inside cgroups in this benchmark?

(I assume in practice the freezing time would be dominated by waiting
for tasks' response, so it'd be good to note this beside this result.)

Thanks,
Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 -next 1/3] cgroup/freezer: Reduce redundant traversal for cgroup_freeze

[Chen Ridong]

On 2024/9/10 3:01, Michal Koutný wrote:
> On Thu, Sep 05, 2024 at 01:41:28PM GMT, Chen Ridong <chenridong@huawei.com> wrote:
>> Whether a cgroup is frozen is determined solely by whether it is set to
>> to be frozen and whether its parent is frozen. Currently, when is cgroup
>> is frozen or unfrozen, it iterates through the entire subtree to freeze
>> or unfreeze its descentdants.
> 
> It's more to maintain the numeric freeze "layers".
> 
>> However, this is unesessary for a cgroup that does not change its
>> effective frozen status.
> 
> True.
> 
>> +static inline void cgroup_update_efreeze(struct cgroup *cgrp)
>> +{
>> +	struct cgroup *parent = cgroup_parent(cgrp);
>> +	bool p_e = false;
>> +
>> +	if (parent)
>> +		p_e = parent->freezer.e_freeze;
>> +
>> +	cgrp->freezer.e_freeze = cgrp->freezer.freeze | p_e;
> 
> Better be || on bools
> 

Thanks you, that is right.

> I'd open code this inside the loop of cgroup_freeze since it is not
> context-less function and it relies on top-down processing.
> 
> Root cgrp cannot be frozen. You can bail out early in the beginning of
> cgroup_freeze() (possibly with a WARN_ON) and then assume parent is
> always valid when iterating.
> 

'cgroup.freeze' is with CFTYPE_NOT_ON_ROOT tag, root cgroup will never 
be frozen, considering root cgroup seems to be unesessary. I will open 
the code inside the loop of cgroup_freeze in next patch.

Thanks
Ridong

> I think maintaining the e_freeze in this "saturated arithmetic" form is
> a sensible change.
> 
> Thanks,
> Michal

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@huawei.com> 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@huawei.com>
>> ---
>>   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

Re: [PATCH v1 -next 3/3] cgroup/freezer: Add freeze selftest

[Chen Ridong]

On 2024/9/10 3:03, Michal Koutný wrote:
> On Thu, Sep 05, 2024 at 01:41:30PM GMT, Chen Ridong <chenridong@huawei.com> wrote:
>> Add selftest to test cgroup.freeze and check cgroup.events state.
> 
> There is already tools/testing/selftests/cgroup/test_freezer.c
> Would you consider adding this as a new test for hierarchies there?
> So that all freezer tests can be run at once and maintained in same style?
> 
> (There are already some creation/removal/event helpers so it shouldn't
> be more difficult than the bash code.)
> 
> Thanks,
> Michal
> 
Okay, I will try.

Thanks,
Ridong

Re: [PATCH v1 -next 0/3] Some optimizations about freezer

[Chen Ridong]

On 2024/9/10 3:04, Michal Koutný wrote:
> Hello.
> 
> On Thu, Sep 05, 2024 at 01:41:27PM GMT, Chen Ridong <chenridong@huawei.com> wrote:
>> We optimized the freezer to reduce redundant loops. We add a selftest to
>> ensure our optimizations cause no harm, and we confirmed that the
>> performance can be improved.
> 
> I think these patches have potential to make the code also cleaner and
> better to understand (the numbers from optimization don't impress me
> that much at the moment).
> 
> I post more comments on the individual patches.
> 
Thanks.

>> We tested the following subtree: D, E, F and G each have n children.
>>          A
>>         / \
>>        B   C
>>      / | \  \
>>      D E F   G
>>     /  |  \   \
>>   1-n 1-n 1-n  0-n
> 
> (Why is the last one different?)
> 
Sorry, this should be same.

>> Our test is to freeze A B C D E F G, and then unfreeze A B C D E F G.
> 
> I would say measuring freezing/unfreezing of only A would give better
> idea about the impact of the change.
> (Going through all descendants manually would unfairly show greater
> improvement.)
> 

I will add this test.
And I only test freezing/unfreezing A, The first patch may not be effective.

>> We measured the elapsed time.
>>
>> 	BEFORE(ns)	AFTER(ns)	SAVED(ns)
>> n=10	142679950	139666014	3,013,936
>> n=100	199832160	192773032	7,059,128
>> n=1000	488595100	414901570	73,693,530
> 
> How many tasks were there inside cgroups in this benchmark?
> 
I didn't put tasks inside cgroups to test. Because this series didn't 
change how the tasks be froze. That only reduced some redundant loop.


> (I assume in practice the freezing time would be dominated by waiting
> for tasks' response, so it'd be good to note this beside this result.)
> 
> Thanks,
> Michal

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

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 716 bytes --]

On Tue, Sep 10, 2024 at 11:08:12AM GMT, Chen Ridong <chenridong@huaweicloud.com> wrote:
> > 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?

Sorry for ambigious wording, I meant that the code you posted appears
to me to be correct (and safe) wrt cgroup tree operations -- thanks to
css_set_lock because frozen bookkeeping is under it (cgroup_mutex would
be too heavy weight for all possible callers of
cgroup_propagate_frozen(), namely cgroup_enter_frozen).

Or do you suggest the current form would need cgroup_mutex
synchronization somehow?

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]