[PATCH v4 10/18] perf trace: Drop targets that died before they were filtered

[Ian Rogers]

sched_process_exit() takes a task out of pids_to_trace and pids_filtered
as it dies, which keeps the maps holding live tasks rather than growing
for the length of the session. It only finds a task there if userspace
put it there first, and userspace writes a pid some time after reading
it:

 - trace__set_filter_pids() writes the thread map and the --filter-pids
   list before the programs are attached, so for those there is nothing
   watching them die at all.
 - trace__set_startup_pids() writes what it enumerated after the attach,
   which is a much smaller gap but still a gap.

A task that dies inside one of those windows is never evicted, because
the delete that would have done it ran while the map had nothing to
delete. Its pid then sits in the map for the rest of the session: it
holds one of a fixed number of entries, and pids are reused, so the
unrelated task that eventually receives it is treated as the target, or
as a task the user asked to leave out and is then silently not reported.

Walk both maps once the last write to them is done and remove the tasks
that are not alive. Sweeping the maps rather than the lists that were
written to them covers every way a pid can get in, including the pid of
perf itself and of the terminal it was started from, which
trace__set_filter_loop_pids() adds without recording anywhere. A task
found alive here and dying later is one sched_process_exit() can see and
evict, so a single pass is enough, and the tasks the BPF programs add
for themselves need nothing: sched_process_fork() inserts a task before
it has run, so it cannot have died beforehand.

Deleting the key the walk is standing on leaves bpf_map__get_next_key()
to resume from a key that is no longer there, which
htab_map_get_next_key() answers with the first key of the whole map,
starting the walk over. The walk therefore only ever stands on a key it
has found alive, which is one it has just decided to keep, so nothing it
deletes is ever the key it would go on to ask from. A key deleted while
the walk stands on nothing is not a problem either: the next request is
for the first key of the map, and the deleted one is no longer it.

sched_process_exit() deletes keys while this runs and can take out the
key the walk is standing on, which there is no way to prevent or to
notice. The walk is bounded at max_entries steps so that it ends however
often that happens: the map holds no more than that many keys, so a walk
that has taken as many steps has either seen them all or been restarted,
and the bound is what stops a walk that keeps being restarted from going
round for ever. Ending on the bound can leave a dead task behind, which
costs one entry of a great many until the session ends, where not ending
would cost the session itself.

A task is taken to be gone when its /proc entry is not there, and also
when /proc reports it as Z or X. Both mean it is in or past do_exit(),
which is where sched_process_exit() runs, so waiting for a zombie to be
reaped before dropping it would only keep a dead task in the map for
longer.

Everything else, including not being able to tell, leaves the task in
the map. The two mistakes are not equal: a dead task left there holds an
entry nothing needs, while a live one taken out stops being traced for
the rest of the session, so a read that fails for perf's own reasons,
being out of file descriptors or of memory, is not read as the task
having exited.

Reading that state means finding the end of the command name, which
do_task_stat() writes out unescaped, unlike /proc/<pid>/status: it holds
whatever the task called itself, ')' and newlines included. The whole of
/proc/<pid>/stat is read rather than a line of it, because a line stops
at the first embedded newline, short of the name's closing ')', and the
last ')' in the file is taken as that one, since no field after the name
has parentheses in it.

Assisted-by: Antigravity:gemini-3.1-pro
Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
---
 tools/perf/builtin-trace.c          | 76 ++++++++++++++++++++++++---
 tools/perf/util/bpf_trace_augment.c | 79 +++++++++++++++++++++++++++++
 tools/perf/util/trace_augment.h     |  6 +++
 3 files changed, 153 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 8a5dbf144540..f2425a291eeb 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -4846,6 +4846,59 @@ static int trace__collect_target_pids(struct trace *trace, struct pid_list *pids
 	return err;
 }
 
+/*
+ * Is the task still running?
+ *
+ * A pid with no /proc entry is gone, and one reported as Z (zombie) or X (dead)
+ * is in or past do_exit(), which is where sched_process_exit() runs. Either way
+ * the BPF programs will never hear about it again.
+ *
+ * Anything that is not one of those, including being unable to tell, is taken
+ * as alive. The two mistakes are not equal: a dead task left in the map holds
+ * an entry that nothing needs, while a live one taken out of it stops being
+ * traced for the rest of the session.
+ */
+static bool trace__task_is_alive(pid_t pid)
+{
+	char path[PATH_MAX];
+	const char *state;
+	char *stat = NULL;
+	bool alive = true;
+	size_t len;
+	int err;
+
+	scnprintf(path, sizeof(path), "%s/%d/stat", procfs__mountpoint(), pid);
+	err = filename__read_str(path, &stat, &len);
+	if (err) {
+		/*
+		 * ENOENT and ESRCH are the task being gone, which is what this
+		 * is looking for. Any other error is perf's own, running out
+		 * of file descriptors or of memory, and says nothing about the
+		 * task.
+		 */
+		return err != -ENOENT && err != -ESRCH;
+	}
+
+	/*
+	 * The state is the field after the command name. do_task_stat() writes
+	 * that name out as it is, so it can hold anything a task cares to call
+	 * itself, ')' and newlines included. Nothing after it has parentheses,
+	 * so the last ')' in the file is the one that closes it; the whole file
+	 * is read rather than a line of it because a line stops at the first of
+	 * those newlines, short of the ')' that is being looked for.
+	 *
+	 * A read that returns something the state cannot be read out of, such
+	 * as the empty result of the task exiting midway through it, leaves the
+	 * task alive by the rule above.
+	 */
+	state = strrchr(stat, ')');
+	if (state != NULL && state[1] == ' ')
+		alive = state[2] != 'Z' && state[2] != 'X';
+
+	free(stat);
+	return alive;
+}
+
 /*
  * Trace the tasks that appeared while perf trace was starting up.
  *
@@ -4868,19 +4921,26 @@ static int trace__collect_target_pids(struct trace *trace, struct pid_list *pids
 static int trace__set_startup_pids(struct trace *trace)
 {
 	struct pid_list pids = {};
-	int err;
+	int err = 0;
 
 	/*
-	 * Nothing to do without a target: 'perf trace -a' does not filter on
-	 * pid at all, and evlist__prepare_workload() keeps a workload blocked
-	 * on a pipe until evlist__start_workload(), well after the attach.
+	 * Only a target is enumerated: 'perf trace -a' does not filter on pid
+	 * at all, and evlist__prepare_workload() keeps a workload blocked on a
+	 * pipe until evlist__start_workload(), well after the attach.
 	 */
-	if (!target__has_task(&trace->opts.target))
-		return 0;
+	if (target__has_task(&trace->opts.target)) {
+		err = trace__collect_target_pids(trace, &pids);
+		if (!err)
+			err = augmented_syscalls__set_target_pids(pids.nr, pids.entries);
+	}
 
-	err = trace__collect_target_pids(trace, &pids);
+	/*
+	 * Every session gets the sweep, target or not: --filter-pids names
+	 * tasks to leave out with no target of its own, and those are written
+	 * to a map of their own, before the attach and with the same race.
+	 */
 	if (!err)
-		err = augmented_syscalls__set_target_pids(pids.nr, pids.entries);
+		err = augmented_syscalls__prune_dead_pids(trace__task_is_alive);
 
 	pid_list__exit(&pids);
 	return err;
diff --git a/tools/perf/util/bpf_trace_augment.c b/tools/perf/util/bpf_trace_augment.c
index 44f30dba5469..4f93f5c062c1 100644
--- a/tools/perf/util/bpf_trace_augment.c
+++ b/tools/perf/util/bpf_trace_augment.c
@@ -255,6 +255,85 @@ int augmented_syscalls__set_target_pids(unsigned int nr, pid_t *pids)
 	return 0;
 }
 
+/*
+ * Remove from a pid keyed map every task that is no longer alive.
+ *
+ * Deleting the key the walk is standing on leaves bpf_map__get_next_key() to
+ * resume from a key that is no longer there, which htab_map_get_next_key()
+ * answers with the first key of the whole map, starting the walk over. The
+ * cursor is therefore only ever moved onto a key that has been found alive,
+ * which is a key this function has just decided to keep, so nothing it does
+ * can send itself back to the beginning.
+ *
+ * sched_process_exit() deletes keys while this runs and can take out the
+ * cursor, which there is no way to prevent or to notice. The walk is bounded
+ * at max_entries steps so that it ends however often that happens: the map
+ * holds no more than that many keys, so a walk that has taken as many steps
+ * has either seen them all or been restarted, and the bound is what stops a
+ * repeatedly restarted walk from going round for ever. Stopping there can
+ * leave a dead task behind, which costs one entry of a great many until the
+ * session ends, where not stopping would cost the session itself.
+ */
+static int prune_dead_map_pids(struct bpf_map *map, bool (*is_alive)(pid_t pid))
+{
+	size_t max_entries = bpf_map__max_entries(map);
+	pid_t cursor, key;
+	bool have_cursor = false;
+
+	for (size_t step = 0; step < max_entries; step++) {
+		int err;
+
+		/* Anything other than success means there is no next key. */
+		if (bpf_map__get_next_key(map, have_cursor ? &cursor : NULL,
+					  &key, sizeof(key)) != 0)
+			break;
+
+		if (is_alive(key)) {
+			cursor = key;
+			have_cursor = true;
+			continue;
+		}
+
+		err = bpf_map__delete_elem(map, &key, sizeof(key), /*flags=*/0);
+		/*
+		 * ENOENT means the key went away while this was running, which
+		 * is sched_process_exit() doing the same job.
+		 */
+		if (err && err != -ENOENT)
+			return err;
+
+		/*
+		 * The cursor stays where it was, which is either a key that is
+		 * still in the map or unset. Unset asks the next step for the
+		 * first key of the map, and the one just deleted is no longer
+		 * it, so the walk moves on either way.
+		 */
+	}
+
+	return 0;
+}
+
+/*
+ * Remove the tasks that are no longer alive from both pid maps.
+ *
+ * has_pids_to_trace and has_pids_filtered are left alone. They say that the
+ * maps are in use rather than that they have anything in them, and clearing
+ * the first would turn a targeted session into a system wide one.
+ */
+int augmented_syscalls__prune_dead_pids(bool (*is_alive)(pid_t pid))
+{
+	int err;
+
+	if (skel == NULL)
+		return 0;
+
+	err = prune_dead_map_pids(skel->maps.pids_to_trace, is_alive);
+	if (!err)
+		err = prune_dead_map_pids(skel->maps.pids_filtered, is_alive);
+
+	return err;
+}
+
 /*
  * Populate syscalls in the BPF syscalls_to_trace map:
  * - not_syscalls: true if '!' prefix was specified (blacklist mode: trace
diff --git a/tools/perf/util/trace_augment.h b/tools/perf/util/trace_augment.h
index ad992f5fa726..87dbf8c46c1e 100644
--- a/tools/perf/util/trace_augment.h
+++ b/tools/perf/util/trace_augment.h
@@ -16,6 +16,7 @@ int augmented_syscalls__create_bpf_output(struct evlist *evlist);
 void augmented_syscalls__setup_bpf_output(void);
 int augmented_syscalls__set_filter_pids(unsigned int nr, pid_t *pids);
 int augmented_syscalls__set_target_pids(unsigned int nr, pid_t *pids);
+int augmented_syscalls__prune_dead_pids(bool (*is_alive)(pid_t pid));
 int augmented_syscalls__set_target_syscalls(unsigned int nr, int *syscall_ids, bool not_syscalls);
 int augmented_syscalls__get_map_fds(int *enter_fd, int *exit_fd, int *beauty_fd);
 struct bpf_program *augmented_syscalls__find_by_title(const char *name);
@@ -55,6 +56,11 @@ static inline int augmented_syscalls__set_target_pids(unsigned int nr __maybe_un
 	return 0;
 }
 
+static inline int augmented_syscalls__prune_dead_pids(bool (*is_alive)(pid_t pid) __maybe_unused)
+{
+	return 0;
+}
+
 static inline int augmented_syscalls__set_target_syscalls(unsigned int nr __maybe_unused,
 							  int *syscall_ids __maybe_unused,
 							  bool not_syscalls __maybe_unused)
-- 
2.55.0.1082.g2b9226bbc0-goog