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

[Bradley Morgan <brads@mainlining.org>]

On 1 November 2026 14:26:51 GMT, Oleg Nesterov <oleg@redhat.com> wrote:
>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.
>

I checked, it is dead code, it IS unnecessary (impossible to reach)

>> @@ -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.
>

--- Thanks!