[v1] cgroup/pids: Avoid spurious event notification

[PATCH -next] cgroup/pids: Avoid spurious event notification

Posted by Xiu Jianfeng 1 year, 4 months ago

From: Xiu Jianfeng <xiujianfeng@huawei.com>

Currently when a process in a group forks and fails due to it's
parent's max restriction, all the cgroups from 'pids_forking' to root
will generate event notifications but only the cgroups from
'pids_over_limit' to root will increase the counter of PIDCG_MAX.

Consider this scenario: there are 4 groups A, B, C,and D, the
relationships are as follows, and user is watching on C.pids.events.

root->A->B->C->D

When a process in D forks and fails due to B.max restriction, the
user will get a spurious event notification because when he wakes up
and reads C.pids.events, he will find that the content has not changed.

To address this issue, only the cgroups from 'pids_over_limit' to root
will have their PIDCG_MAX counter increased and event notifications
generated.

Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
 kernel/cgroup/pids.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/kernel/cgroup/pids.c b/kernel/cgroup/pids.c
index f5cb0ec45b9d..1c8c9f76ff0e 100644
--- a/kernel/cgroup/pids.c
+++ b/kernel/cgroup/pids.c
@@ -244,7 +244,6 @@ static void pids_event(struct pids_cgroup *pids_forking,
 		       struct pids_cgroup *pids_over_limit)
 {
 	struct pids_cgroup *p = pids_forking;
-	bool limit = false;
 
 	/* Only log the first time limit is hit. */
 	if (atomic64_inc_return(&p->events_local[PIDCG_FORKFAIL]) == 1) {
@@ -257,15 +256,11 @@ static void pids_event(struct pids_cgroup *pids_forking,
 	    cgrp_dfl_root.flags & CGRP_ROOT_PIDS_LOCAL_EVENTS)
 		return;
 
-	for (; parent_pids(p); p = parent_pids(p)) {
-		if (p == pids_over_limit) {
-			limit = true;
-			atomic64_inc(&p->events_local[PIDCG_MAX]);
-			cgroup_file_notify(&p->events_local_file);
-		}
-		if (limit)
-			atomic64_inc(&p->events[PIDCG_MAX]);
+	atomic64_inc(&pids_over_limit->events_local[PIDCG_MAX]);
+	cgroup_file_notify(&pids_over_limit->events_local_file);
 
+	for (p = pids_over_limit; parent_pids(p); p = parent_pids(p)) {
+		atomic64_inc(&p->events[PIDCG_MAX]);
 		cgroup_file_notify(&p->events_file);
 	}
 }
-- 
2.34.1

Re: [PATCH -next] cgroup/pids: Avoid spurious event notification

Posted by Michal Koutný 1 year, 4 months ago

Hello.

On Mon, Jul 29, 2024 at 10:58:24AM GMT, Xiu Jianfeng <xiujianfeng@huaweicloud.com> wrote:
> To address this issue, only the cgroups from 'pids_over_limit' to root
> will have their PIDCG_MAX counter increased and event notifications
> generated.
> 

For completeness here

Fixes: 385a635cacfe0 ("cgroup/pids: Make event counters hierarchical")

> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
>  kernel/cgroup/pids.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)



> @@ -257,15 +256,11 @@ static void pids_event(struct pids_cgroup *pids_forking,
>  	    cgrp_dfl_root.flags & CGRP_ROOT_PIDS_LOCAL_EVENTS)
>  		return;
>  
> -	for (; parent_pids(p); p = parent_pids(p)) {
> -		if (p == pids_over_limit) {
> -			limit = true;
> -			atomic64_inc(&p->events_local[PIDCG_MAX]);
> -			cgroup_file_notify(&p->events_local_file);
> -		}
> -		if (limit)
> -			atomic64_inc(&p->events[PIDCG_MAX]);
> +	atomic64_inc(&pids_over_limit->events_local[PIDCG_MAX]);
> +	cgroup_file_notify(&pids_over_limit->events_local_file);
>  
> +	for (p = pids_over_limit; parent_pids(p); p = parent_pids(p)) {
> +		atomic64_inc(&p->events[PIDCG_MAX]);
>  		cgroup_file_notify(&p->events_file);
>  	}

When I look at it applied altogther, there's one extra notification
(heritage of forkfail events), it should be fixed with:

--- a/kernel/cgroup/pids.c
+++ b/kernel/cgroup/pids.c
@@ -251,10 +251,11 @@ static void pids_event(struct pids_cgroup *pids_forking,
                pr_cont_cgroup_path(p->css.cgroup);
                pr_cont("\n");
        }
-       cgroup_file_notify(&p->events_local_file);
        if (!cgroup_subsys_on_dfl(pids_cgrp_subsys) ||
-           cgrp_dfl_root.flags & CGRP_ROOT_PIDS_LOCAL_EVENTS)
+           cgrp_dfl_root.flags & CGRP_ROOT_PIDS_LOCAL_EVENTS) {
+               cgroup_file_notify(&p->events_local_file);
                return;
+       }
 
        atomic64_inc(&pids_over_limit->events_local[PIDCG_MAX]);
        cgroup_file_notify(&pids_over_limit->events_local_file);

Besides that it makes sense to me.

Thanks,
Michal

Re: [PATCH -next] cgroup/pids: Avoid spurious event notification

Posted by Xiu Jianfeng 1 year, 4 months ago


On 2024/7/30 0:07, Michal Koutný wrote:
> Hello.
> 
> On Mon, Jul 29, 2024 at 10:58:24AM GMT, Xiu Jianfeng <xiujianfeng@huaweicloud.com> wrote:
>> To address this issue, only the cgroups from 'pids_over_limit' to root
>> will have their PIDCG_MAX counter increased and event notifications
>> generated.
>>
> 
> For completeness here
> 
> Fixes: 385a635cacfe0 ("cgroup/pids: Make event counters hierarchical")>
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
>> ---
>>  kernel/cgroup/pids.c | 13 ++++---------
>>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> 
> 
>> @@ -257,15 +256,11 @@ static void pids_event(struct pids_cgroup *pids_forking,
>>  	    cgrp_dfl_root.flags & CGRP_ROOT_PIDS_LOCAL_EVENTS)
>>  		return;
>>  
>> -	for (; parent_pids(p); p = parent_pids(p)) {
>> -		if (p == pids_over_limit) {
>> -			limit = true;
>> -			atomic64_inc(&p->events_local[PIDCG_MAX]);
>> -			cgroup_file_notify(&p->events_local_file);
>> -		}
>> -		if (limit)
>> -			atomic64_inc(&p->events[PIDCG_MAX]);
>> +	atomic64_inc(&pids_over_limit->events_local[PIDCG_MAX]);
>> +	cgroup_file_notify(&pids_over_limit->events_local_file);
>>  
>> +	for (p = pids_over_limit; parent_pids(p); p = parent_pids(p)) {
>> +		atomic64_inc(&p->events[PIDCG_MAX]);
>>  		cgroup_file_notify(&p->events_file);
>>  	}
> 
> When I look at it applied altogther, there's one extra notification
> (heritage of forkfail events), it should be fixed with:
> 
> --- a/kernel/cgroup/pids.c
> +++ b/kernel/cgroup/pids.c
> @@ -251,10 +251,11 @@ static void pids_event(struct pids_cgroup *pids_forking,
>                 pr_cont_cgroup_path(p->css.cgroup);
>                 pr_cont("\n");
>         }
> -       cgroup_file_notify(&p->events_local_file);
>         if (!cgroup_subsys_on_dfl(pids_cgrp_subsys) ||
> -           cgrp_dfl_root.flags & CGRP_ROOT_PIDS_LOCAL_EVENTS)
> +           cgrp_dfl_root.flags & CGRP_ROOT_PIDS_LOCAL_EVENTS) {
> +               cgroup_file_notify(&p->events_local_file);
>                 return;
> +       }

Thanks, looks good, will do in the next version.

>  
>         atomic64_inc(&pids_over_limit->events_local[PIDCG_MAX]);
>         cgroup_file_notify(&pids_over_limit->events_local_file);
> 
> Besides that it makes sense to me.
> 
> Thanks,
> Michal