Re: [PATCH 1/4] perf lock contention: Factor out lock_contention_get_name()

[Arnaldo Carvalho de Melo]

Em Wed, Feb 01, 2023 at 09:04:52PM -0800, Namhyung Kim escreveu:
> The lock_contention_get_name() returns a name for the lock stat entry
> based on the current aggregation mode.  As it's called sequentially in a
> single thread, it can return the address of a static buffer for symbol
> and offset of the caller.
> 
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
>  tools/perf/util/bpf_lock_contention.c | 113 ++++++++++++++------------
>  1 file changed, 63 insertions(+), 50 deletions(-)
> 
> diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> index 4902ac331f41..967ce168f163 100644
> --- a/tools/perf/util/bpf_lock_contention.c
> +++ b/tools/perf/util/bpf_lock_contention.c
> @@ -163,9 +163,68 @@ int lock_contention_stop(void)
>  	return 0;
>  }
>  
> +static const char *lock_contention_get_name(struct lock_contention *con,
> +					    struct contention_key *key,
> +					    u64 *stack_trace)
> +{
> +	int idx = 0;
> +	u64 addr;
> +	const char *name = "";
> +	static char name_buf[KSYM_NAME_LEN];
> +	struct symbol *sym;
> +	struct map *kmap;
> +	struct machine *machine = con->machine;
> +
> +	if (con->aggr_mode == LOCK_AGGR_TASK) {
> +		struct contention_task_data task;
> +		struct thread *t;
> +		int pid = key->aggr_key;
> +		int task_fd = bpf_map__fd(skel->maps.task_data);
> +

I'm processing this as-is, but please consider to reduce the number of
lines by declaring variables where they are needed, for instance, the
't' variable is only used inside the next if block, so it could have
been:

> +		/* do not update idle comm which contains CPU number */
> +		if (pid) {
> +			bpf_map_lookup_elem(task_fd, &pid, &task);
> +			t = __machine__findnew_thread(machine, /*pid=*/-1, pid);

+			struct thread *t = __machine__findnew_thread(machine, /*pid=*/-1, pid);

But since __machine__findnew_thread() can fail, please check for that
and send a new patch series... Humm, you're just factoring out, the
problem was there before, so, to make progress, I'll process it as is
and later we can add fixes, can you please look into that?

> +			thread__set_comm(t, task.comm, /*timestamp=*/0);

thread__set_comm() can fail as well.

> +			name = task.comm;
> +		}
> +		return name;
> +	}
> +
> +	if (con->aggr_mode == LOCK_AGGR_ADDR) {
> +		sym = machine__find_kernel_symbol(machine, key->aggr_key, &kmap);
> +		if (sym)
> +			name = sym->name;

Here you check :-)

> +		return name;
> +	}
> +
> +	/* LOCK_AGGR_CALLER: skip lock internal functions */
> +	while (machine__is_lock_function(machine, stack_trace[idx]) &&
> +	       idx < con->max_stack - 1)
> +		idx++;
> +
> +	addr = stack_trace[idx];
> +	sym = machine__find_kernel_symbol(machine, addr, &kmap);
> +
> +	if (sym) {
> +		unsigned long offset;
> +
> +		offset = kmap->map_ip(kmap, addr) - sym->start;
> +
> +		if (offset == 0)
> +			return sym->name;
> +
> +		snprintf(name_buf, sizeof(name_buf), "%s+%#lx", sym->name, offset);
> +	} else {
> +		snprintf(name_buf, sizeof(name_buf), "%#lx", (unsigned long)addr);
> +	}
> +
> +	return name_buf;
> +}
> +
>  int lock_contention_read(struct lock_contention *con)
>  {
> -	int fd, stack, task_fd, err = 0;
> +	int fd, stack, err = 0;
>  	struct contention_key *prev_key, key;
>  	struct contention_data data = {};
>  	struct lock_stat *st = NULL;
> @@ -175,7 +234,6 @@ int lock_contention_read(struct lock_contention *con)
>  
>  	fd = bpf_map__fd(skel->maps.lock_stat);
>  	stack = bpf_map__fd(skel->maps.stacks);
> -	task_fd = bpf_map__fd(skel->maps.task_data);
>  
>  	con->lost = skel->bss->lost;
>  
> @@ -195,9 +253,6 @@ int lock_contention_read(struct lock_contention *con)
>  
>  	prev_key = NULL;
>  	while (!bpf_map_get_next_key(fd, prev_key, &key)) {
> -		struct map *kmap;
> -		struct symbol *sym;
> -		int idx = 0;
>  		s32 stack_id;
>  
>  		/* to handle errors in the loop body */
> @@ -219,61 +274,19 @@ int lock_contention_read(struct lock_contention *con)
>  		st->flags = data.flags;
>  		st->addr = key.aggr_key;
>  
> -		if (con->aggr_mode == LOCK_AGGR_TASK) {
> -			struct contention_task_data task;
> -			struct thread *t;
> -			int pid = key.aggr_key;
> -
> -			/* do not update idle comm which contains CPU number */
> -			if (st->addr) {
> -				bpf_map_lookup_elem(task_fd, &pid, &task);
> -				t = __machine__findnew_thread(machine, /*pid=*/-1, pid);
> -				thread__set_comm(t, task.comm, /*timestamp=*/0);
> -			}
> -			goto next;
> -		}
> -
> -		if (con->aggr_mode == LOCK_AGGR_ADDR) {
> -			sym = machine__find_kernel_symbol(machine, st->addr, &kmap);
> -			if (sym)
> -				st->name = strdup(sym->name);
> -			goto next;
> -		}
> -
>  		stack_id = key.aggr_key;
>  		bpf_map_lookup_elem(stack, &stack_id, stack_trace);
>  
> -		/* skip lock internal functions */
> -		while (machine__is_lock_function(machine, stack_trace[idx]) &&
> -		       idx < con->max_stack - 1)
> -			idx++;
> -
> -		st->addr = stack_trace[idx];
> -		sym = machine__find_kernel_symbol(machine, st->addr, &kmap);
> -
> -		if (sym) {
> -			unsigned long offset;
> -			int ret = 0;
> -
> -			offset = kmap->map_ip(kmap, st->addr) - sym->start;
> -
> -			if (offset)
> -				ret = asprintf(&st->name, "%s+%#lx", sym->name, offset);
> -			else
> -				st->name = strdup(sym->name);
> -
> -			if (ret < 0 || st->name == NULL)
> -				break;
> -		} else if (asprintf(&st->name, "%#lx", (unsigned long)st->addr) < 0) {
> +		st->name = strdup(lock_contention_get_name(con, &key, stack_trace));
> +		if (st->name == NULL)
>  			break;
> -		}
>  
>  		if (con->save_callstack) {
>  			st->callstack = memdup(stack_trace, stack_size);
>  			if (st->callstack == NULL)
>  				break;
>  		}
> -next:
> +
>  		hlist_add_head(&st->hash_entry, con->result);
>  		prev_key = &key;
>  
> -- 
> 2.39.1.456.gfc5497dd1b-goog
> 

-- 

- Arnaldo