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

[xiujianfeng]

Hi,

Friendly ping, more comment as below.

On 2024/7/3 14:59, xiujianfeng wrote:
> 
> 
> 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);
> }
>

or should the for loop be changed to the following?

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(&pt->events[PIDCG_MAX]);
    cgroup_file_notify(&p->events_file);
}

The current behaviour is quite different from other subsys, such as
memcg, that make me confused, maybe I am missing something.

it's appreciated if anyone could respond.