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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5a29e13b-7956-4f68-8c39-92183e5ed0ca@huaweicloud.com>
Date: Tue, 10 Sep 2024 11:08:12 +0800
From: Chen Ridong <chenridong@...weicloud.com>
To: Michal Koutný <mkoutny@...e.com>,
 Chen Ridong <chenridong@...wei.com>
Cc: tj@...nel.org, lizefan.x@...edance.com, hannes@...xchg.org,
 longman@...hat.com, adityakali@...gle.com, sergeh@...nel.org,
 cgroups@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 -next 2/3] cgroup/freezer: Reduce redundant propagation
 for cgroup_propagate_frozen



On 2024/9/10 3:02, Michal Koutný wrote:
> On Thu, Sep 05, 2024 at 01:41:29PM GMT, Chen Ridong <chenridong@...wei.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@...wei.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