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

[Xiu Jianfeng <xiujianfeng@huaweicloud.com>]

On 2024/7/25 17:38, Michal Koutný wrote:
> Hello Jianfeng.
> 
> On Tue, Jul 16, 2024 at 11:27:39AM GMT, xiujianfeng <xiujianfeng@huawei.com> wrote:
>> On 2024/7/3 14:59, xiujianfeng wrote:
> ...
>>>         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?
> 
> Two generic notes:
> - event notifications can be rate limited, so users won't necessarily
>   see every change,
> - upon notification it's better to read the event counter/status anyway
>   to base a response on it.
> 
> But your remark is justified, there is no reason in this case for
> "spurious" event notification. It's an omission from v3 version of the
> patch when there had been also pids.events:max.imposed (that'd trigger
> events from D up to the root, it's only internal PIDCG_FORKFAIL now).
> 
> The upwards traversal loop can be simplified and fixed with only
> PIDCG_MAX exposed. Can you send it as a separate patch please?

Hi Michal,

Thanks for your feedback. and I'm sorry I forgot to reply this thread
after sending the patch.

> 
> (Apologies for late response, somehow I didn't see your e-mails.)
> 
> Michal