On 2024/7/25 17:38, Michal Koutný wrote:
> Hello Jianfeng.
>
> On Tue, Jul 16, 2024 at 11:27:39AM GMT, xiujianfeng <xiujianfeng@xxxxxxxxxx> 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
