* Re: [PATCH] taskstats: Avoid counting an exiting thread twice in TGID queries
2026-10-31 5:24 [PATCH] taskstats: Avoid counting an exiting thread twice in TGID queries Baek Kyu, Kim
@ 2026-10-31 15:23 ` Bradley Morgan
2026-11-01 14:26 ` Oleg Nesterov
2026-11-07 7:03 ` [PATCH v2] taskstats: account exiting threads once in TGID reports Baek Kyu, Kim
2 siblings, 0 replies; 5+ messages in thread
From: Bradley Morgan @ 2026-10-31 15:23 UTC (permalink / raw)
To: pocj8ur4in; +Cc: akpm, bsingharora, linux-kernel, oleg
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!
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] taskstats: Avoid counting an exiting thread twice in TGID queries
2026-10-31 5:24 [PATCH] taskstats: Avoid counting an exiting thread twice in TGID queries Baek Kyu, Kim
2026-10-31 15:23 ` Bradley Morgan
@ 2026-11-01 14:26 ` Oleg Nesterov
2026-11-01 14:53 ` Bradley Morgan
2026-11-07 7:03 ` [PATCH v2] taskstats: account exiting threads once in TGID reports Baek Kyu, Kim
2 siblings, 1 reply; 5+ messages in thread
From: Oleg Nesterov @ 2026-11-01 14:26 UTC (permalink / raw)
To: Baek Kyu, Kim; +Cc: Balbir Singh, Andrew Morton, linux-kernel, Bradley Morgan
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2] taskstats: account exiting threads once in TGID reports
2026-10-31 5:24 [PATCH] taskstats: Avoid counting an exiting thread twice in TGID queries Baek Kyu, Kim
2026-10-31 15:23 ` Bradley Morgan
2026-11-01 14:26 ` Oleg Nesterov
@ 2026-11-07 7:03 ` Baek Kyu, Kim
2 siblings, 0 replies; 5+ messages in thread
From: Baek Kyu, Kim @ 2026-11-07 7:03 UTC (permalink / raw)
To: Balbir Singh
Cc: Baek Kyu, Kim, Andrew Morton, Oleg Nesterov, Bradley Morgan,
linux-kernel
A TGID query can count an exiting thread twice after its contribution
reaches the cache, or miss a zombie thread whose contribution has not
been cached. The final TGID exit notification can also be sent before
other exiting threads finish cache accounting.
Track whether each exiting thread has contributed to the cache, and
use that state when collecting TGID statistics. Defer the final exit
notification until all exiting threads finish cache accounting. This
keeps TGID queries and the final notification consistent across thread
exit races.
In an arm64 QEMU/KVM guest, forced exit races confirmed that TGID
queries count a thread once and the final notification includes both
exiting threads. The existing taskstats selftest passed.
Fixes: ad4ecbcba728 ("[PATCH] delay accounting taskstats interface send tgid once")
Link: https://lore.kernel.org/all/asaKCo5lwB3nzWNM@redhat.com/
Signed-off-by: Baek Kyu, Kim <pocj8ur4in@gmail.com>
---
Changes in v2:
- Remove the fork-time reset: a task cannot fork after taskstats_exit().
- Drop the exit_state check: an unaccounted zombie still belongs in the
TGID query, including for a single-threaded process.
- Defer the final PID/AGROUP and TGID notification until every exiting
thread has finished cache accounting; keep both records in one message.
- Use task_struct::atomic_flags rather than adding a new task_struct member.
- Mark failed cache allocation as finished so a deferred notification
is not left pending.
v1: https://lists.openwall.net/linux-kernel/2026/10/31/267
include/linux/sched.h | 9 ++++
include/linux/sched/signal.h | 5 +++
kernel/taskstats.c | 104 +++++++++++++++++++++++++++++++++----------
3 files changed, 95 insertions(+), 23 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d35ae49a991f7..257e6028c88c1 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1875,6 +1875,8 @@ static __always_inline bool is_user_task(struct task_struct *task)
#define PFA_SPEC_IB_DISABLE 5 /* Indirect branch speculation restricted */
#define PFA_SPEC_IB_FORCE_DISABLE 6 /* Indirect branch speculation permanently restricted */
#define PFA_SPEC_SSB_NOEXEC 7 /* Speculative Store Bypass clear on execve() */
+#define PFA_TASKSTATS_TGID_ACCOUNTED 8 /* Included in signal->stats */
+#define PFA_TASKSTATS_TGID_SKIPPED 9 /* No TGID cache at taskstats_exit */
#define TASK_PFA_TEST(name, func) \
static inline bool task_##func(struct task_struct *p) \
@@ -1913,6 +1915,13 @@ TASK_PFA_CLEAR(SPEC_IB_DISABLE, spec_ib_disable)
TASK_PFA_TEST(SPEC_IB_FORCE_DISABLE, spec_ib_force_disable)
TASK_PFA_SET(SPEC_IB_FORCE_DISABLE, spec_ib_force_disable)
+#ifdef CONFIG_TASKSTATS
+TASK_PFA_TEST(TASKSTATS_TGID_ACCOUNTED, tgid_stats_accounted)
+TASK_PFA_SET(TASKSTATS_TGID_ACCOUNTED, tgid_stats_accounted)
+TASK_PFA_TEST(TASKSTATS_TGID_SKIPPED, tgid_stats_skipped)
+TASK_PFA_SET(TASKSTATS_TGID_SKIPPED, tgid_stats_skipped)
+#endif
+
static inline void
current_restore_flags(unsigned long orig_flags, unsigned long flags)
{
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index efa1e2d6c7a6b..b6f4e24e1552e 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -14,6 +14,9 @@
#include <linux/mm_types.h>
#include <asm/ptrace.h>
+struct listener_list;
+struct sk_buff;
+
/*
* Types defining task->signal and task->sighand and APIs using them:
*/
@@ -221,6 +224,8 @@ struct signal_struct {
#endif
#ifdef CONFIG_TASKSTATS
struct taskstats *stats;
+ struct sk_buff *taskstats_pending_skb;
+ struct listener_list *taskstats_pending_listeners;
#endif
#ifdef CONFIG_AUDIT
unsigned audit_tty;
diff --git a/kernel/taskstats.c b/kernel/taskstats.c
index f31df72f0e9df..3e1f5a44ee62f 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 (task_tgid_stats_accounted(tsk))
continue;
tgid_stats_add_task(stats, tsk, now_ns);
@@ -280,20 +280,48 @@ static int fill_stats_for_tgid(pid_t tgid, struct taskstats *stats)
return rc;
}
-static void fill_tgid_exit(struct task_struct *tsk)
+static bool tgid_stats_complete(struct task_struct *tsk)
{
+ struct task_struct *thread;
+ bool complete = true;
+
+ if (atomic_read(&tsk->signal->live))
+ return false;
+
+ rcu_read_lock();
+ for_each_thread(tsk, thread) {
+ if (!task_tgid_stats_accounted(thread) &&
+ !task_tgid_stats_skipped(thread)) {
+ complete = false;
+ break;
+ }
+ }
+ rcu_read_unlock();
+
+ return complete;
+}
+
+static struct sk_buff *finish_tgid_exit(struct task_struct *tsk, bool account,
+ struct listener_list **pending_listeners)
+{
+ struct sk_buff *pending = NULL;
unsigned long flags;
- u64 now_ns;
spin_lock_irqsave(&tsk->sighand->siglock, flags);
- if (!tsk->signal->stats)
- goto ret;
-
- now_ns = ktime_get_ns();
- tgid_stats_add_task(tsk->signal->stats, tsk, now_ns);
-ret:
+ if (account) {
+ tgid_stats_add_task(tsk->signal->stats, tsk, ktime_get_ns());
+ task_set_tgid_stats_accounted(tsk);
+ } else {
+ task_set_tgid_stats_skipped(tsk);
+ }
+ if (tgid_stats_complete(tsk)) {
+ pending = tsk->signal->taskstats_pending_skb;
+ *pending_listeners = tsk->signal->taskstats_pending_listeners;
+ tsk->signal->taskstats_pending_skb = NULL;
+ tsk->signal->taskstats_pending_listeners = NULL;
+ }
spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
- return;
+ return pending;
}
static int add_del_listener(pid_t pid, const struct cpumask *mask, int isadd)
@@ -581,13 +609,34 @@ static struct taskstats *taskstats_tgid_alloc(struct task_struct *tsk)
return stats;
}
+static void send_tgid_notification(struct sk_buff *skb,
+ struct task_struct *tsk,
+ struct listener_list *listeners)
+{
+ struct taskstats *stats;
+
+ stats = mk_reply(skb, TASKSTATS_TYPE_TGID,
+ task_tgid_nr_ns(tsk, &init_pid_ns));
+ if (!stats) {
+ nlmsg_free(skb);
+ return;
+ }
+
+ memcpy(stats, tsk->signal->stats, sizeof(*stats));
+ stats->version = TASKSTATS_VERSION;
+ send_cpu_listeners(skb, listeners);
+}
+
/* Send pid data out on exit */
void taskstats_exit(struct task_struct *tsk, int group_dead)
{
int rc;
struct listener_list *listeners;
struct taskstats *stats;
- struct sk_buff *rep_skb;
+ struct sk_buff *rep_skb, *pending = NULL;
+ struct listener_list *pending_listeners = NULL;
+ unsigned long flags;
+ bool complete;
size_t size;
int is_thread_group;
@@ -603,17 +652,17 @@ void taskstats_exit(struct task_struct *tsk, int group_dead)
if (is_thread_group) {
/* PID + STATS + TGID + STATS */
size = 2 * size;
- /* fill the tsk->signal->stats structure */
- fill_tgid_exit(tsk);
}
+ /* A failed cache allocation must not hold a pending TGID reply forever. */
+ pending = finish_tgid_exit(tsk, is_thread_group, &pending_listeners);
listeners = raw_cpu_ptr(&listener_array);
if (list_empty(&listeners->list))
- return;
+ goto out_pending;
rc = prepare_reply(NULL, TASKSTATS_CMD_NEW, &rep_skb, size);
if (rc < 0)
- return;
+ goto out_pending;
stats = mk_reply(rep_skb, TASKSTATS_TYPE_PID,
task_pid_nr_ns(tsk, &init_pid_ns));
@@ -630,19 +679,28 @@ void taskstats_exit(struct task_struct *tsk, int group_dead)
if (!is_thread_group || !group_dead)
goto send;
- stats = mk_reply(rep_skb, TASKSTATS_TYPE_TGID,
- task_tgid_nr_ns(tsk, &init_pid_ns));
- if (!stats)
- goto err;
+ spin_lock_irqsave(&tsk->sighand->siglock, flags);
+ complete = tgid_stats_complete(tsk);
+ if (!complete) {
+ tsk->signal->taskstats_pending_skb = rep_skb;
+ tsk->signal->taskstats_pending_listeners = listeners;
+ }
+ spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
+ if (!complete)
+ goto out_pending;
- memcpy(stats, tsk->signal->stats, sizeof(*stats));
- stats->version = TASKSTATS_VERSION;
+ send_tgid_notification(rep_skb, tsk, listeners);
+ goto out_pending;
send:
send_cpu_listeners(rep_skb, listeners);
+out_pending:
+ if (pending)
+ send_tgid_notification(pending, tsk, pending_listeners);
return;
err:
nlmsg_free(rep_skb);
+ goto out_pending;
}
static const struct genl_ops taskstats_ops[] = {
--
2.55.0
^ permalink raw reply related [flat|nested] 5+ messages in thread