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

[Michal Koutný]

Hello.

On Mon, Jul 29, 2024 at 10:58:24AM GMT, Xiu Jianfeng <xiujianfeng@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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

Attachment: signature.asc
Description: PGP signature