If there are no listeners, taskstats_exit_send() just returns because
taskstats_exit_alloc() didn't allocate *tidstats. This is wrong, each
sub-thread should do fill_tgid_exit() on exit, otherwise its ->delays
is not recorded in ->signal->stats and lost.
Q: We don't send TASKSTATS_TYPE_AGGR_TGID when single-threaded process
exits. Is it good? How can the listener figure out that it was actually
a process exit, not sub-thread?
Signed-off-by: Oleg Nesterov <[email protected]>
--- STATS/kernel/taskstats.c~2_send 2006-10-30 23:47:46.000000000 +0300
+++ STATS/kernel/taskstats.c 2006-10-31 00:14:42.000000000 +0300
@@ -446,10 +446,9 @@ void taskstats_exit_send(struct task_str
int is_thread_group;
struct nlattr *na;
- if (!family_registered || !tidstats)
+ if (!family_registered)
return;
- rc = 0;
/*
* Size includes space for nested attributes
*/
@@ -457,8 +456,15 @@ void taskstats_exit_send(struct task_str
nla_total_size(sizeof(struct taskstats)) + nla_total_size(0);
is_thread_group = (tsk->signal->stats != NULL);
- if (is_thread_group)
- size = 2 * size; /* PID + STATS + TGID + STATS */
+ if (is_thread_group) {
+ /* PID + STATS + TGID + STATS */
+ size = 2 * size;
+ /* fill the tsk->signal->stats structure */
+ fill_tgid_exit(tsk);
+ }
+
+ if (!tidstats)
+ return;
rc = prepare_reply(NULL, TASKSTATS_CMD_NEW, &rep_skb, &reply, size);
if (rc < 0)
@@ -478,11 +484,8 @@ void taskstats_exit_send(struct task_str
goto send;
/*
- * tsk has/had a thread group so fill the tsk->signal->stats structure
* Doesn't matter if tsk is the leader or the last group member leaving
*/
-
- fill_tgid_exit(tsk);
if (!group_dead)
goto send;
Oleg Nesterov wrote:
> If there are no listeners, taskstats_exit_send() just returns because
> taskstats_exit_alloc() didn't allocate *tidstats. This is wrong, each
> sub-thread should do fill_tgid_exit() on exit, otherwise its ->delays
> is not recorded in ->signal->stats and lost.
Good catch. Thanks for the detailed look at the delay accounting code.
>
> Q: We don't send TASKSTATS_TYPE_AGGR_TGID when single-threaded process
> exits. Is it good? How can the listener figure out that it was actually
> a process exit, not sub-thread?
We had a detailed discussion on this on lkml earlier. The overhead of
sending essentially the same data twice (once as AGGR_TGID and once as
PID) was deemed too heavy esp. as the taskstats structure size grew.
Also, single threaded exit is a common case.
Using process events, its possible for user space to distinguish single
threaded process exits.
>
> Signed-off-by: Oleg Nesterov <[email protected]>
Acked-by: Shailabh Nagar <[email protected]>
>
> --- STATS/kernel/taskstats.c~2_send 2006-10-30 23:47:46.000000000 +0300
> +++ STATS/kernel/taskstats.c 2006-10-31 00:14:42.000000000 +0300
> @@ -446,10 +446,9 @@ void taskstats_exit_send(struct task_str
> int is_thread_group;
> struct nlattr *na;
>
> - if (!family_registered || !tidstats)
> + if (!family_registered)
> return;
>
> - rc = 0;
> /*
> * Size includes space for nested attributes
> */
> @@ -457,8 +456,15 @@ void taskstats_exit_send(struct task_str
> nla_total_size(sizeof(struct taskstats)) + nla_total_size(0);
>
> is_thread_group = (tsk->signal->stats != NULL);
> - if (is_thread_group)
> - size = 2 * size; /* PID + STATS + TGID + STATS */
> + if (is_thread_group) {
> + /* PID + STATS + TGID + STATS */
> + size = 2 * size;
> + /* fill the tsk->signal->stats structure */
> + fill_tgid_exit(tsk);
> + }
> +
> + if (!tidstats)
> + return;
>
> rc = prepare_reply(NULL, TASKSTATS_CMD_NEW, &rep_skb, &reply, size);
> if (rc < 0)
> @@ -478,11 +484,8 @@ void taskstats_exit_send(struct task_str
> goto send;
>
> /*
> - * tsk has/had a thread group so fill the tsk->signal->stats structure
> * Doesn't matter if tsk is the leader or the last group member leaving
> */
> -
> - fill_tgid_exit(tsk);
> if (!group_dead)
> goto send;
>
>
On 10/30, Shailabh Nagar wrote:
>
> Oleg Nesterov wrote:
> >
> >Q: We don't send TASKSTATS_TYPE_AGGR_TGID when single-threaded process
> >exits. Is it good? How can the listener figure out that it was actually
> >a process exit, not sub-thread?
>
> We had a detailed discussion on this on lkml earlier. The overhead of
> sending essentially the same data twice (once as AGGR_TGID and once as
> PID) was deemed too heavy esp. as the taskstats structure size grew.
> Also, single threaded exit is a common case.
>
> Using process events, its possible for user space to distinguish single
> threaded process exits.
Ok, I see.
The taskstats's code is very clean and understandable, the only thing
I can't get is: why these listeners are per-cpu? It is very easy to add
'int exited_on_this_cpu' to struct taskstats.
Probaly this was done to filter out unneeded events (cpusets) ? In that
case it seems better to add cpumask_t to 'struct listener' but have a
single listener_array list.
Thanks!
Oleg.