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.
