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?
(Apologies for late response, somehow I didn't see your e-mails.)
Michal