Re: [PATCH v5 2/5] cgroup/pids: Make event counters hierarchical

[xiujianfeng <xiujianfeng@xxxxxxxxxx>]

On 2024/5/21 17:21, Michal Koutný wrote:
> The pids.events file should honor the hierarchy, so make the events
> propagate from their origin up to the root on the unified hierarchy. The
> legacy behavior remains non-hierarchical.
> 
> Signed-off-by: Michal Koutný <mkoutny@xxxxxxxx>
> --
[...]
> diff --git a/kernel/cgroup/pids.c b/kernel/cgroup/pids.c
> index a557f5c8300b..c09b744d548c 100644
> --- a/kernel/cgroup/pids.c
> +++ b/kernel/cgroup/pids.c
> @@ -238,6 +238,34 @@ static void pids_cancel_attach(struct cgroup_taskset *tset)
>  	}
>  }
>  
> +static void pids_event(struct pids_cgroup *pids_forking,
> +		       struct pids_cgroup *pids_over_limit)
> +{
> +	struct pids_cgroup *p = pids_forking;
> +	bool limit = false;
> +
> +	for (; parent_pids(p); p = parent_pids(p)) {
> +		/* Only log the first time limit is hit. */
> +		if (atomic64_inc_return(&p->events[PIDCG_FORKFAIL]) == 1) {
> +			pr_info("cgroup: fork rejected by pids controller in ");
> +			pr_cont_cgroup_path(p->css.cgroup);
> +			pr_cont("\n");
> +		}
> +		cgroup_file_notify(&p->events_file);
> +
> +		if (!cgroup_subsys_on_dfl(pids_cgrp_subsys) ||
> +		    cgrp_dfl_root.flags & CGRP_ROOT_PIDS_LOCAL_EVENTS)
> +			break;
> +
> +		if (p == pids_over_limit)
> +			limit = true;
> +		if (limit)
> +			atomic64_inc(&p->events[PIDCG_MAX]);
> +
> +		cgroup_file_notify(&p->events_file);

Hi Michal,

I have doubts about this code. To better illustrate the problem, I am
posting the final code here.

static void pids_event(struct pids_cgroup *pids_forking,
                       struct pids_cgroup *pids_over_limit)
{
...
        cgroup_file_notify(&p->events_local_file);
        if (!cgroup_subsys_on_dfl(pids_cgrp_subsys) ||
            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]);

                cgroup_file_notify(&p->events_file);
        }
}

Consider this scenario: there are 4 groups A, B, C,and D. The
relationships are as follows, the latter is the child of the former:

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

Then the user is polling on C.pids.events. When a process in D forks and
fails due to B.max restrictions(pids_forking is D, and pids_over_limit
is B), the user is awakened. However, when the user reads C.pids.events,
he will find that the content has not changed. because the 'limit' is
set to true started from B, and C.pids.events shows as below:

seq_printf(sf, "max %lld\n", (s64)atomic64_read(&events[PIDCG_MAX]));

Wouldn't this behavior confuse the user? Should the code to be changed
to this?

if (limit) {
      atomic64_inc(&p->events[PIDCG_MAX]);
      cgroup_file_notify(&p->events_file);
}