| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 2 Feb 2023 09:53:09 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Jiri Olsa <jolsa@...nel.org>, Ingo Molnar <mingo@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
LKML <linux-kernel@...r.kernel.org>,
Ian Rogers <irogers@...gle.com>,
Adrian Hunter <adrian.hunter@...el.com>,
linux-perf-users@...r.kernel.org, Song Liu <song@...nel.org>,
Hao Luo <haoluo@...gle.com>, bpf@...r.kernel.org
Subject: Re: [PATCH 1/4] perf lock contention: Factor out
lock_contention_get_name()
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@...nel.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
>
--
- Arnaldo
Powered by blists - more mailing lists