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

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

[Baek Kyu, Kim]

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.

Mark a task when its contribution reaches the cache, under the same
siglock used by the TGID query. Exclude marked tasks from the live scan.
Leave a task eligible for the scan if no cache exists, and clear the
marker when a new task is copied during fork.

Fixes: ad4ecbcba728 ("[PATCH] delay accounting taskstats interface send tgid once")
Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>
---
In an arm64 QEMU guest, holding the interval after cache accounting but
before exit_state changed made a 100 ms worker appear twice in a TGID CPU
query. The baseline reported 208000 us for 8000 us of the querying thread
plus 100000 us of the worker; the change reported 112000 us for 12000 us
plus 100000 us. Natural race frequency and other architectures were not
measured; KASAN and KCSAN were not enabled.

 include/linux/sched.h | 5 +++++
 kernel/fork.c         | 3 +++
 kernel/taskstats.c    | 7 ++++---
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index d35ae49a991f..359ee982323c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1457,6 +1457,11 @@ struct task_struct {
 	struct task_delay_info		*delays;
 #endif
 
+#ifdef CONFIG_TASKSTATS
+	/* Protected by sighand->siglock. */
+	bool				taskstats_tgid_accounted;
+#endif
+
 #ifdef CONFIG_FAULT_INJECTION
 	int				make_it_fail;
 	unsigned int			fail_nth;
diff --git a/kernel/fork.c b/kernel/fork.c
index 33760ece3bb3..76d9abcc5ca8 100644
--- 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
 	p->flags &= ~(PF_SUPERPRIV | PF_WQ_WORKER | PF_IDLE | PF_NO_SETAFFINITY);
 	p->flags |= PF_FORKNOEXEC;
 	INIT_LIST_HEAD(&p->children);
diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index f31df72f0e9d..dd9054de761f 100644
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -245,8 +245,8 @@ static int fill_stats_for_tgid(pid_t tgid, struct taskstats *stats)
 	u64 now_ns;
 
 	/*
-	 * Add additional stats from live tasks except zombie thread group
-	 * leaders who are already counted with the dead tasks
+	 * Add stats from tasks not yet included in the dead-task cache.
+	 * Accounting can finish before exit_state is set.
 	 */
 	rcu_read_lock();
 	first = find_task_by_vpid(tgid);
@@ -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;
 
 		tgid_stats_add_task(stats, tsk, now_ns);
@@ -291,6 +291,7 @@ static void fill_tgid_exit(struct task_struct *tsk)
 
 	now_ns = ktime_get_ns();
 	tgid_stats_add_task(tsk->signal->stats, tsk, now_ns);
+	tsk->taskstats_tgid_accounted = true;
 ret:
 	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
 	return;
-- 
2.55.0

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

[Bradley Morgan]

On 31 October 2026 05:24:51 GMT, "Baek Kyu, Kim" <pocj8ur4in@gmail.com>
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.
>
>Mark a task when its contribution reaches the cache, under the same
>siglock used by the TGID query. Exclude marked tasks from the live scan.
>Leave a task eligible for the scan if no cache exists, and clear the
>marker when a new task is copied during fork.

This part of the changelog would need to go, see below.

>
>Fixes: ad4ecbcba728 ("[PATCH] delay accounting taskstats interface send tgid once")

Thx, Oleg already suggested this kinda thing

Link: https://lore.kernel.org/all/asaKCo5lwB3nzWNM@redhat.com/ would be good. Or closes?

>Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>
>---
>In an arm64 QEMU guest, holding the interval after cache accounting but
>before exit_state changed made a 100 ms worker appear twice in a TGID CPU
>query. The baseline reported 208000 us for 8000 us of the querying thread
>plus 100000 us of the worker; the change reported 112000 us for 12000 us
>plus 100000 us. Natural race frequency and other architectures were not
>measured; KASAN and KCSAN were not enabled.
>
> include/linux/sched.h | 5 +++++
> kernel/fork.c         | 3 +++
> kernel/taskstats.c    | 7 ++++---
> 3 files changed, 12 insertions(+), 3 deletions(-)
>
>diff --git a/include/linux/sched.h b/include/linux/sched.h
>index d35ae49a991f..359ee982323c 100644
>--- a/include/linux/sched.h
>+++ b/include/linux/sched.h
>@@ -1457,6 +1457,11 @@ struct task_struct {
> 	struct task_delay_info		*delays;
> #endif
> 
>+#ifdef CONFIG_TASKSTATS
>+	/* Protected by sighand->siglock. */
>+	bool				taskstats_tgid_accounted;
>+#endif
>+
> #ifdef CONFIG_FAULT_INJECTION
> 	int				make_it_fail;
> 	unsigned int			fail_nth;
>diff --git a/kernel/fork.c b/kernel/fork.c
>index 33760ece3bb3..76d9abcc5ca8 100644
>--- 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

Id like to pray that this is dead, and I'm not just being a idiot.

The flag is set in fill_tgid_exit() and the task that already is past
taskstats_exit() in do_exit, it can't call fork() again, dup_task_struct()
copies from current, so I'd reckon that this is useless.

> 	p->flags &= ~(PF_SUPERPRIV | PF_WQ_WORKER | PF_IDLE | PF_NO_SETAFFINITY);
> 	p->flags |= PF_FORKNOEXEC;
> 	INIT_LIST_HEAD(&p->children);
>diff --git a/kernel/taskstats.c b/kernel/taskstats.c
>index f31df72f0e9d..dd9054de761f 100644
>--- a/kernel/taskstats.c
>+++ b/kernel/taskstats.c
>@@ -245,8 +245,8 @@ static int fill_stats_for_tgid(pid_t tgid, struct taskstats *stats)
> 	u64 now_ns;
> 
> 	/*
>-	 * Add additional stats from live tasks except zombie thread group
>-	 * leaders who are already counted with the dead tasks
>+	 * Add stats from tasks not yet included in the dead-task cache.
>+	 * Accounting can finish before exit_state is set.
> 	 */
> 	rcu_read_lock();
> 	first = find_task_by_vpid(tgid);
>@@ -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;
> 
> 		tgid_stats_add_task(stats, tsk, now_ns);
>@@ -291,6 +291,7 @@ static void fill_tgid_exit(struct task_struct *tsk)
> 
> 	now_ns = ktime_get_ns();
> 	tgid_stats_add_task(tsk->signal->stats, tsk, now_ns);
>+	tsk->taskstats_tgid_accounted = true;
> ret:
> 	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
> 	return;
>


Btw: you've sent 8 patches today, please stop spamming please.

--- Thanks!

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

[Oleg Nesterov]

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.

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

[Bradley Morgan]

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!