diff mbox series

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

Message ID 20230202050455.2187592-2-namhyung@kernel.org (mailing list archive)
State Handled Elsewhere
Delegated to: BPF
Headers show
Series perf lock contention: Improve aggr x filter combination (v1) | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-3 success Logs for build for aarch64 with llvm-17
bpf/vmtest-bpf-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-6 success Logs for build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-7 success Logs for llvm-toolchain
bpf/vmtest-bpf-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-9 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for test_maps on aarch64 with llvm-17
bpf/vmtest-bpf-VM_Test-11 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-14 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-15 fail Logs for test_progs on aarch64 with llvm-17
bpf/vmtest-bpf-VM_Test-16 fail Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-18 fail Logs for test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-19 fail Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-20 success Logs for test_progs_no_alu32 on aarch64 with llvm-17
bpf/vmtest-bpf-VM_Test-21 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-25 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-17
bpf/vmtest-bpf-VM_Test-26 success Logs for test_progs_no_alu32_parallel on s390x with gcc
bpf/vmtest-bpf-VM_Test-27 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-29 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-30 success Logs for test_progs_parallel on aarch64 with llvm-17
bpf/vmtest-bpf-VM_Test-31 success Logs for test_progs_parallel on s390x with gcc
bpf/vmtest-bpf-VM_Test-32 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-33 success Logs for test_progs_parallel on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-34 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-35 success Logs for test_verifier on aarch64 with llvm-17
bpf/vmtest-bpf-VM_Test-36 fail Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-37 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-38 success Logs for test_verifier on x86_64 with llvm-17

Commit Message

Namhyung Kim Feb. 2, 2023, 5:04 a.m. UTC
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@kernel.org>
---
 tools/perf/util/bpf_lock_contention.c | 113 ++++++++++++++------------
 1 file changed, 63 insertions(+), 50 deletions(-)

Comments

Arnaldo Carvalho de Melo Feb. 2, 2023, 12:53 p.m. UTC | #1
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@kernel.org>
> ---
>  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
>
diff mbox series

Patch

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);
+
+		/* 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);
+			thread__set_comm(t, task.comm, /*timestamp=*/0);
+			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;
+		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;