Re: [PATCH] taskstats: Avoid counting an exiting thread twice in TGID queries

[Oleg Nesterov <oleg@redhat.com>]

On 10/31, Baek Kyu, Kim wrote:
>
> fill_tgid_exit() adds a thread's contribution to signal->stats before
> exit_notify() sets exit_state. A TGID query in that interval copies the
> cached contribution, then scans the same thread because exit_state is
> still clear. The thread's contribution is counted twice.

I forgot everything about this code, quite possibly I am wrong but let
me add some comments anyway...

Yes, known problem. Plus afaics fill_stats_for_tgid() can also miss an
exiting task, see below.

Another problem is that taskstats_exit(group_exit => true) can miss other
threads which have not called taskstats_exit() yet and report TGID/AGROUP.
Afaics this patch doesn't try to fix this.

> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2202,6 +2202,9 @@ __latent_entropy struct task_struct *copy_process(
>  		goto bad_fork_cleanup_count;
>
>  	delayacct_tsk_init(p);	/* Must remain after dup_task_struct() */
> +#ifdef CONFIG_TASKSTATS
> +	p->taskstats_tgid_accounted = false;
> +#endif

I agree with Bradley, this looks unnecessary.

> @@ -261,7 +261,7 @@ static int fill_stats_for_tgid(pid_t tgid, struct taskstats *stats)
>
>  	now_ns = ktime_get_ns();
>  	for_each_thread(first, tsk) {
> -		if (tsk->exit_state)
> +		if (tsk->exit_state || tsk->taskstats_tgid_accounted)
>  			continue;

If we add taskstats_tgid_accounted, why do we need to check exit_state?

In the multithreaded case, if taskstats_tgid_accounted is true, then
exit_state should be true too (ignoring the kmem_cache_zalloc() failures).

In the single threaded case this looks simply wrong. The exiting single
threaded task will not call fill_tgid_exit(). If cmd_attr_tgid() is called
after exit_notify(), it will miss that zombie.


And... Perhaps it would be better to use ->atomic_flags rather than add
yet another member to task_struct?

Oleg.