[PATHC v3 -next 0/3] Some optimizations about freezer

[PATHC v3 -next 0/3] Some optimizations about freezer

[Chen Ridong]

I optimized the freezer to reduce redundant loops, and I add helper
to make code concise.

The following subtree was tested to prove whether my optimizations are
effective.
   0
 / | \ \
A  B C  1
      / | \ \
     A  B C  2
        .....
	         n
               / | \
              A  B C

I tested by following steps:
1. freeze 0
2. unfreeze 0
3. freeze 0
4. freeze 1

And I measured the elapsed time(ns).

n=10
	freeze 0	unfreeze 0	freeze 0	freeze 1
BEFORE	106179390	94016050	110423650	95063770
AFTER	96473608	91054188	94936398	93198510

n=50
	freeze 0	unfreeze 0	freeze 0	freeze 1
BEFORE	109506660	105643800	105970220	96948940
AFTER	105244651	97357482	97517358	88466266

n=100
	freeze 0	unfreeze 0	freeze 0	freeze 1
BEFORE	127944210	122049330	120988900	101232850
AFTER	117298106	107034146	105696895	91977833

As shown above, after optimizations, it can save elapsed time.
By freezing 0 and subsequently freezing 1, the elapsed time is consistent,
indicating that my optimizations are highly effective.

---
v3:
- fix build warnings reported-by kernel test robot.

v2:
- open code inside the loop of cgroup_freeze instead of inline function.
- add helper to make code concise.
- remove selftest script(There are hierarchy test in test_freeze.c, I
  think that is enough for this series).

Chen Ridong (3):
  cgroup/freezer: Reduce redundant traversal for cgroup_freeze
  cgroup/freezer: Add cgroup CGRP_FROZEN flag update helper
  cgroup/freezer: Reduce redundant propagation for
    cgroup_propagate_frozen

 include/linux/cgroup-defs.h |   6 +-
 kernel/cgroup/freezer.c     | 110 ++++++++++++++++++------------------
 2 files changed, 59 insertions(+), 57 deletions(-)

-- 
2.34.1

[PATHC v3 -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     | 30 ++++++++++++++----------------
 2 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 47ae4c4d924c..dd1ecab99eeb 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -397,7 +397,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..188d5f2aeb5a 100644
--- a/kernel/cgroup/freezer.c
+++ b/kernel/cgroup/freezer.c
@@ -260,8 +260,10 @@ void cgroup_freezer_migrate_task(struct task_struct *task,
 void cgroup_freeze(struct cgroup *cgrp, bool freeze)
 {
 	struct cgroup_subsys_state *css;
+	struct cgroup *parent;
 	struct cgroup *dsct;
 	bool applied = false;
+	bool old_e;
 
 	lockdep_assert_held(&cgroup_mutex);
 
@@ -282,22 +284,18 @@ 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);
+		/*
+		 * e_freeze is affected by parent's e_freeze and dst's freeze.
+		 * 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;
+		parent = cgroup_parent(dsct);
+		dsct->freezer.e_freeze = (dsct->freezer.freeze ||
+					  parent->freezer.e_freeze);
+		if (dsct->freezer.e_freeze == old_e) {
+			css = css_rightmost_descendant(css);
+			continue;
 		}
 
 		/*
-- 
2.34.1

[PATHC v3 -next 2/3] cgroup/freezer: Add cgroup CGRP_FROZEN flag update helper

[Chen Ridong]

Add help to update cgroup CGRP_FROZEN flag. Both cgroup_propagate_frozen
and cgroup_update_frozen functions update CGRP_FROZEN flag, this makes
code concise.

Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
 kernel/cgroup/freezer.c | 67 ++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 35 deletions(-)

diff --git a/kernel/cgroup/freezer.c b/kernel/cgroup/freezer.c
index 188d5f2aeb5a..bf1690a167dd 100644
--- a/kernel/cgroup/freezer.c
+++ b/kernel/cgroup/freezer.c
@@ -8,6 +8,28 @@
 
 #include <trace/events/cgroup.h>
 
+/*
+ * Update CGRP_FROZEN of cgroup.flag
+ * Return true if flags is updated; false if flags has no change
+ */
+static bool cgroup_update_frozen_flag(struct cgroup *cgrp, bool frozen)
+{
+	lockdep_assert_held(&css_set_lock);
+
+	/* Already there? */
+	if (test_bit(CGRP_FROZEN, &cgrp->flags) == frozen)
+		return false;
+
+	if (frozen)
+		set_bit(CGRP_FROZEN, &cgrp->flags);
+	else
+		clear_bit(CGRP_FROZEN, &cgrp->flags);
+
+	cgroup_file_notify(&cgrp->events_file);
+	TRACE_CGROUP_PATH(notify_frozen, cgrp, frozen);
+	return true;
+}
+
 /*
  * Propagate the cgroup frozen state upwards by the cgroup tree.
  */
@@ -24,24 +46,16 @@ static void cgroup_propagate_frozen(struct cgroup *cgrp, bool frozen)
 	while ((cgrp = cgroup_parent(cgrp))) {
 		if (frozen) {
 			cgrp->freezer.nr_frozen_descendants += desc;
-			if (!test_bit(CGRP_FROZEN, &cgrp->flags) &&
-			    test_bit(CGRP_FREEZE, &cgrp->flags) &&
-			    cgrp->freezer.nr_frozen_descendants ==
-			    cgrp->nr_descendants) {
-				set_bit(CGRP_FROZEN, &cgrp->flags);
-				cgroup_file_notify(&cgrp->events_file);
-				TRACE_CGROUP_PATH(notify_frozen, cgrp, 1);
-				desc++;
-			}
+			if (!test_bit(CGRP_FREEZE, &cgrp->flags) ||
+			    (cgrp->freezer.nr_frozen_descendants !=
+			    cgrp->nr_descendants))
+				continue;
 		} else {
 			cgrp->freezer.nr_frozen_descendants -= desc;
-			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++;
-			}
 		}
+
+		if (cgroup_update_frozen_flag(cgrp, frozen))
+			desc++;
 	}
 }
 
@@ -53,8 +67,6 @@ void cgroup_update_frozen(struct cgroup *cgrp)
 {
 	bool frozen;
 
-	lockdep_assert_held(&css_set_lock);
-
 	/*
 	 * If the cgroup has to be frozen (CGRP_FREEZE bit set),
 	 * and all tasks are frozen and/or stopped, let's consider
@@ -63,24 +75,9 @@ void cgroup_update_frozen(struct cgroup *cgrp)
 	frozen = test_bit(CGRP_FREEZE, &cgrp->flags) &&
 		cgrp->freezer.nr_frozen_tasks == __cgroup_task_count(cgrp);
 
-	if (frozen) {
-		/* Already there? */
-		if (test_bit(CGRP_FROZEN, &cgrp->flags))
-			return;
-
-		set_bit(CGRP_FROZEN, &cgrp->flags);
-	} else {
-		/* Already there? */
-		if (!test_bit(CGRP_FROZEN, &cgrp->flags))
-			return;
-
-		clear_bit(CGRP_FROZEN, &cgrp->flags);
-	}
-	cgroup_file_notify(&cgrp->events_file);
-	TRACE_CGROUP_PATH(notify_frozen, cgrp, frozen);
-
-	/* Update the state of ancestor cgroups. */
-	cgroup_propagate_frozen(cgrp, frozen);
+	/* If flags is updated, update the state of ancestor cgroups. */
+	if (cgroup_update_frozen_flag(cgrp, frozen))
+		cgroup_propagate_frozen(cgrp, frozen);
 }
 
 /*
-- 
2.34.1

[PATHC v3 -next 3/3] cgroup/freezer: Reduce redundant propagation for cgroup_propagate_frozen

[Chen Ridong]

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

Additionally, cgroup_propagate_frozen is not only used to update the
ancestor state but also to update itself. This approach can make the code
clearer and significantly simplify cgroup_update_frozen.

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

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;
+	struct cgroup *parent;
 	/*
 	 * 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);
 		}
 
-		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;
 	}
 }
 
@@ -75,9 +82,7 @@ void cgroup_update_frozen(struct cgroup *cgrp)
 	frozen = test_bit(CGRP_FREEZE, &cgrp->flags) &&
 		cgrp->freezer.nr_frozen_tasks == __cgroup_task_count(cgrp);
 
-	/* If flags is updated, update the state of ancestor cgroups. */
-	if (cgroup_update_frozen_flag(cgrp, frozen))
-		cgroup_propagate_frozen(cgrp, frozen);
+	cgroup_propagate_frozen(cgrp, frozen);
 }
 
 /*
-- 
2.34.1

Re: [PATHC v3 -next 0/3] Some optimizations about freezer

[chenridong]

On 2024/9/15 15:13, Chen Ridong wrote:
> I optimized the freezer to reduce redundant loops, and I add helper
> to make code concise.
> 
> The following subtree was tested to prove whether my optimizations are
> effective.
>     0
>   / | \ \
> A  B C  1
>        / | \ \
>       A  B C  2
>          .....
> 	         n
>                 / | \
>                A  B C
> 
> I tested by following steps:
> 1. freeze 0
> 2. unfreeze 0
> 3. freeze 0
> 4. freeze 1
> 
> And I measured the elapsed time(ns).
> 
> n=10
> 	freeze 0	unfreeze 0	freeze 0	freeze 1
> BEFORE	106179390	94016050	110423650	95063770
> AFTER	96473608	91054188	94936398	93198510
> 
> n=50
> 	freeze 0	unfreeze 0	freeze 0	freeze 1
> BEFORE	109506660	105643800	105970220	96948940
> AFTER	105244651	97357482	97517358	88466266
> 
> n=100
> 	freeze 0	unfreeze 0	freeze 0	freeze 1
> BEFORE	127944210	122049330	120988900	101232850
> AFTER	117298106	107034146	105696895	91977833
> 
> As shown above, after optimizations, it can save elapsed time.
> By freezing 0 and subsequently freezing 1, the elapsed time is consistent,
> indicating that my optimizations are highly effective.
> 
> ---
> v3:
> - fix build warnings reported-by kernel test robot.
> 
> v2:
> - open code inside the loop of cgroup_freeze instead of inline function.
> - add helper to make code concise.
> - remove selftest script(There are hierarchy test in test_freeze.c, I
>    think that is enough for this series).
> 
> Chen Ridong (3):
>    cgroup/freezer: Reduce redundant traversal for cgroup_freeze
>    cgroup/freezer: Add cgroup CGRP_FROZEN flag update helper
>    cgroup/freezer: Reduce redundant propagation for
>      cgroup_propagate_frozen
> 
>   include/linux/cgroup-defs.h |   6 +-
>   kernel/cgroup/freezer.c     | 110 ++++++++++++++++++------------------
>   2 files changed, 59 insertions(+), 57 deletions(-)
> 

Friendly ping.

Best regards,
Ridong

Re: [PATHC v3 -next 0/3] Some optimizations about freezer

[Tejun Heo]

On Mon, Sep 23, 2024 at 11:37:14AM +0800, chenridong wrote:
> Friendly ping.

Will apply after the merge window.

Thanks.

-- 
tejun

Re: [PATHC v3 -next 0/3] Some optimizations about freezer

[Chen Ridong]

On 2024/9/23 23:36, Tejun Heo wrote:
> On Mon, Sep 23, 2024 at 11:37:14AM +0800, chenridong wrote:
>> Friendly ping.
> 
> Will apply after the merge window.
> 
> Thanks.
> 
Thank you.

Best regards,
Ridong

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

[Michal Koutný]

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

On Sun, Sep 15, 2024 at 07:13:05AM GMT, Chen Ridong <chenridong@huawei.com> wrote:
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> ---
>  include/linux/cgroup-defs.h |  2 +-
>  kernel/cgroup/freezer.c     | 30 ++++++++++++++----------------
>  2 files changed, 15 insertions(+), 17 deletions(-)

Reviewed-by: Michal Koutný <mkoutny@suse.com>


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

Re: [PATHC v3 -next 2/3] cgroup/freezer: Add cgroup CGRP_FROZEN flag update helper

[Michal Koutný]

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

On Sun, Sep 15, 2024 at 07:13:06AM GMT, Chen Ridong <chenridong@huawei.com> wrote:
> Add help to update cgroup CGRP_FROZEN flag. Both cgroup_propagate_frozen
> and cgroup_update_frozen functions update CGRP_FROZEN flag, this makes
> code concise.
> 
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> ---
>  kernel/cgroup/freezer.c | 67 ++++++++++++++++++++---------------------
>  1 file changed, 32 insertions(+), 35 deletions(-)

Reviewed-by: Michal Koutný <mkoutny@suse.com>


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

Re: [PATHC v3 -next 3/3] cgroup/freezer: Reduce redundant propagation for cgroup_propagate_frozen

[Michal Koutný]

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

On Sun, Sep 15, 2024 at 07:13:07AM GMT, Chen Ridong <chenridong@huawei.com> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATHC v3 -next 3/3] cgroup/freezer: Reduce redundant propagation for cgroup_propagate_frozen

[Chen Ridong]

On 2024/9/26 1:46, Michal Koutný wrote:
> On Sun, Sep 15, 2024 at 07:13:07AM GMT, Chen Ridong <chenridong@huawei.com> 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.)
> 
Thank you, Michal.

I sorry I missed this situation.
If unfreezing a cgroup, it seems it has to propagate to the top.

After consideration, I modify this function.
the following is acceptable?

/*
  * Propagate the cgroup frozen state upwards by the cgroup tree.
  */
static void cgroup_propagate_frozen(struct cgroup *cgrp, bool frozen)
{
	int deta = 0;
	struct cgroup *parent;
	/*
	 * case1: If the new state is frozen, some freezing ancestor cgroups 
may change
	 * their state too, depending on if all their descendants are frozen.
	 *
	 * case2: unfrozen, all ancestor cgroups are forced into the non-frozen 
state.
	 */
	for (; cgrp; cgrp = cgroup_parent(cgrp)) {
		if (frozen) {
			/* 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))
				break;			
			/* No change, stop propagate */
			if (!cgroup_update_frozen_flag(cgrp, frozen))
				break;
			deta = cgrp->freezer.nr_frozen_descendants + 1;
		} else {
			/* case2: have to propagate all ancestor */
			if (cgroup_update_frozen_flag(cgrp, frozen))
				deta++;
		}

		parent = cgroup_parent(cgrp);
		parent->freezer.nr_frozen_descendants += deta;
	}
}

Best regards,
Ridong

Re: [PATHC v3 -next 3/3] cgroup/freezer: Reduce redundant propagation for cgroup_propagate_frozen

[Chen Ridong]

On 2024/9/27 17:46, Chen Ridong wrote:
> 
> 
> On 2024/9/26 1:46, Michal Koutný wrote:
>> On Sun, Sep 15, 2024 at 07:13:07AM GMT, Chen Ridong 
>> <chenridong@huawei.com> 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.)
>>
> Thank you, Michal.
> 
> I sorry I missed this situation.
> If unfreezing a cgroup, it seems it has to propagate to the top.
> 
> After consideration, I modify this function.
> the following is acceptable?
> 
> /*
>   * Propagate the cgroup frozen state upwards by the cgroup tree.
>   */
> static void cgroup_propagate_frozen(struct cgroup *cgrp, bool frozen)
> {
>      int deta = 0;
>      struct cgroup *parent;
>      /*
>       * case1: If the new state is frozen, some freezing ancestor 
> cgroups may change
>       * their state too, depending on if all their descendants are frozen.
>       *
>       * case2: unfrozen, all ancestor cgroups are forced into the 
> non-frozen state.
>       */
>      for (; cgrp; cgrp = cgroup_parent(cgrp)) {
>          if (frozen) {
>              /* 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))
>                  break;
>              /* No change, stop propagate */
>              if (!cgroup_update_frozen_flag(cgrp, frozen))
>                  break;
>              deta = cgrp->freezer.nr_frozen_descendants + 1;
>          } else {
>              /* case2: have to propagate all ancestor */
>              if (cgroup_update_frozen_flag(cgrp, frozen))
>                  deta++;
>          }
> 
>          parent = cgroup_parent(cgrp);
>          parent->freezer.nr_frozen_descendants += deta;
>      }
> }
> 
> Best regards,
> Ridong
> 
Hi, Michal, Do you think this can be acceptable?
I don't have a better idea, If you have a better a idea, please let me know.

Best regards,
Ridong