Re: [PATCH] perf sched timehist: decode process names of processes in zombie state

[Namhyung Kim <namhyung@kernel.org>]

Hello,

On Wed, Jul 09, 2025 at 10:31:20AM -0400, Anubhav Shelat wrote:
> Previously when running perf trace timehist --state, when recording
> processes in the zombie state the process name would not be decoded
> properly and appears with just the PID:
> 
> 1140057.412177 [0006]  Mutter Input Th[3139/3104]          0.956      0.019      0.041      S
> 1140057.412222 [0012]  :1248612[1248612]                   0.000      0.000      0.332      Z
> 1140057.412275 [0004]  <idle>                              0.052      0.052      0.953      I
> 1140057.412284 [0008]  <idle>                              0.070      0.070      0.932      I
> 1140057.412333 [0004]  KMS thread[3126/3104]               0.953      0.112      0.058      S
> 
> Now some extra processing has been added to decode the process name:
> 
> 1140057.412177 [0006]  Mutter Input Th[3139/3104]          0.956      0.019      0.041      S
> 1140057.412222 [0012]  sleep[1248612]                      0.000      0.000      0.332      Z
> 1140057.412275 [0004]  <idle>                              0.052      0.052      0.953      I
> 1140057.412284 [0008]  <idle>                              0.070      0.070      0.932      I
> 1140057.412333 [0004]  KMS thread[3126/3104]               0.953      0.112      0.058      S

Thanks for the fix.  A nitpick below.

> 
> Signed-off-by: Anubhav Shelat <ashelat@redhat.com>
> ---
>  tools/perf/builtin-sched.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 26ece6e9bfd1..e1a931341bc8 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -2023,11 +2023,15 @@ static int comm_width = 30;
>  static char *timehist_get_commstr(struct thread *thread)
>  {
>  	static char str[32];
> -	const char *comm = thread__comm_str(thread);
> +	char *comm = (char *) thread__comm_str(thread);
> +	char *empty = (char *) "";
>  	pid_t tid = thread__tid(thread);
>  	pid_t pid = thread__pid(thread);
>  	int n;
>  
> +	if (*comm == ':')
> +		comm = empty;
> +
>  	if (pid == 0)
>  		n = scnprintf(str, sizeof(str), "%s", comm);
>  
> @@ -2147,6 +2151,8 @@ static void timehist_print_sample(struct perf_sched *sched,
>  	struct thread_runtime *tr = thread__priv(thread);
>  	const char *next_comm = evsel__strval(evsel, sample, "next_comm");
>  	const u32 next_pid = evsel__intval(evsel, sample, "next_pid");
> +	const char *prev_comm = evsel__strval(evsel, sample, "prev_comm");
> +	char *comm_str = timehist_get_commstr(thread);
>  	u32 max_cpus = sched->max_cpu.cpu + 1;
>  	char tstr[64];
>  	char nstr[30];
> @@ -2173,8 +2179,15 @@ static void timehist_print_sample(struct perf_sched *sched,
>  		}
>  		printf(" ");
>  	}
> -
> -	printf(" %-*s ", comm_width, timehist_get_commstr(thread));
> +	
> +	/* if a process is in zombie state commstr will start with a '[' and 
> +	 * we need to do extra processing to decode the process name */
> +	if (*comm_str == '[') {
> +		printf(" %s", evsel__strval(evsel, sample, "prev_comm"));
> +		printf("%-*s ", comm_width - (int) strlen(prev_comm), comm_str);
> +	}
> +	else
> +		printf(" %-*s ", comm_width, comm_str);

What about just updating thread comm if it's missing?

	if (!thread__comm_set(thread)) {
		const char *prev_comm = evsel__strval(evsel, sample, "prev_comm");
		thread__set_comm(thread, prev_comm, sample->time);
	}
	
	printf(" %-%s ", comm_width, timehist_get_commstr(thread));

Thanks,
Namhyung

>  
>  	if (sched->show_prio)
>  		printf(" %-*s ", MAX_PRIO_STR_LEN, timehist_get_priostr(evsel, thread, sample));
> -- 
> 2.50.0
>