From: Jiri Olsa <olsajiri@gmail.com>
To: Ian Rogers <irogers@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Namhyung Kim <namhyung@kernel.org>,
Adrian Hunter <adrian.hunter@intel.com>,
Song Liu <song@kernel.org>, Ming Wang <wangming01@loongson.cn>,
Ravi Bangoria <ravi.bangoria@amd.com>,
Huacai Chen <chenhuacai@kernel.org>,
Kan Liang <kan.liang@linux.intel.com>,
K Prateek Nayak <kprateek.nayak@amd.com>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
bpf@vger.kernel.org
Subject: Re: [PATCH v1] perf env: Avoid recursively taking env->bpf_progs.lock
Date: Wed, 3 Jan 2024 10:25:57 +0100 [thread overview]
Message-ID: <ZZUoJfGbPwLcFCnb@krava> (raw)
In-Reply-To: <20231207014655.1252484-1-irogers@google.com>
On Wed, Dec 06, 2023 at 05:46:55PM -0800, Ian Rogers wrote:
> Add variants of perf_env__insert_bpf_prog_info, perf_env__insert_btf
> and perf_env__find_btf prefixed with __ to indicate the
> env->bpf_progs.lock is assumed held. Call these variants when the lock
> is held to avoid recursively taking it and potentially having a thread
> deadlock with itself.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> Previously this patch was part of a larger set:
> https://lore.kernel.org/lkml/20231127220902.1315692-51-irogers@google.com/
> ---
> tools/perf/util/bpf-event.c | 8 +++---
> tools/perf/util/bpf-event.h | 12 ++++-----
> tools/perf/util/env.c | 53 +++++++++++++++++++++++--------------
> tools/perf/util/env.h | 4 +++
> tools/perf/util/header.c | 8 +++---
> 5 files changed, 51 insertions(+), 34 deletions(-)
>
lgtm
Acked-by: Jiri Olsa <jolsa@kernel.org>
jirka
> diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
> index 830711cae30d..3573e0b7ef3e 100644
> --- a/tools/perf/util/bpf-event.c
> +++ b/tools/perf/util/bpf-event.c
> @@ -545,9 +545,9 @@ int evlist__add_bpf_sb_event(struct evlist *evlist, struct perf_env *env)
> return evlist__add_sb_event(evlist, &attr, bpf_event__sb_cb, env);
> }
>
> -void bpf_event__print_bpf_prog_info(struct bpf_prog_info *info,
> - struct perf_env *env,
> - FILE *fp)
> +void __bpf_event__print_bpf_prog_info(struct bpf_prog_info *info,
> + struct perf_env *env,
> + FILE *fp)
> {
> __u32 *prog_lens = (__u32 *)(uintptr_t)(info->jited_func_lens);
> __u64 *prog_addrs = (__u64 *)(uintptr_t)(info->jited_ksyms);
> @@ -563,7 +563,7 @@ void bpf_event__print_bpf_prog_info(struct bpf_prog_info *info,
> if (info->btf_id) {
> struct btf_node *node;
>
> - node = perf_env__find_btf(env, info->btf_id);
> + node = __perf_env__find_btf(env, info->btf_id);
> if (node)
> btf = btf__new((__u8 *)(node->data),
> node->data_size);
> diff --git a/tools/perf/util/bpf-event.h b/tools/perf/util/bpf-event.h
> index 1bcbd4fb6c66..e2f0420905f5 100644
> --- a/tools/perf/util/bpf-event.h
> +++ b/tools/perf/util/bpf-event.h
> @@ -33,9 +33,9 @@ struct btf_node {
> int machine__process_bpf(struct machine *machine, union perf_event *event,
> struct perf_sample *sample);
> int evlist__add_bpf_sb_event(struct evlist *evlist, struct perf_env *env);
> -void bpf_event__print_bpf_prog_info(struct bpf_prog_info *info,
> - struct perf_env *env,
> - FILE *fp);
> +void __bpf_event__print_bpf_prog_info(struct bpf_prog_info *info,
> + struct perf_env *env,
> + FILE *fp);
> #else
> static inline int machine__process_bpf(struct machine *machine __maybe_unused,
> union perf_event *event __maybe_unused,
> @@ -50,9 +50,9 @@ static inline int evlist__add_bpf_sb_event(struct evlist *evlist __maybe_unused,
> return 0;
> }
>
> -static inline void bpf_event__print_bpf_prog_info(struct bpf_prog_info *info __maybe_unused,
> - struct perf_env *env __maybe_unused,
> - FILE *fp __maybe_unused)
> +static inline void __bpf_event__print_bpf_prog_info(struct bpf_prog_info *info __maybe_unused,
> + struct perf_env *env __maybe_unused,
> + FILE *fp __maybe_unused)
> {
>
> }
> diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
> index c68b7a004f29..cfdacbf29456 100644
> --- a/tools/perf/util/env.c
> +++ b/tools/perf/util/env.c
> @@ -22,15 +22,20 @@ struct perf_env perf_env;
> #include "bpf-utils.h"
> #include <bpf/libbpf.h>
>
> -void perf_env__insert_bpf_prog_info(struct perf_env *env,
> - struct bpf_prog_info_node *info_node)
> +void perf_env__insert_bpf_prog_info(struct perf_env *env, struct bpf_prog_info_node *info_node)
> +{
> + down_write(&env->bpf_progs.lock);
> + __perf_env__insert_bpf_prog_info(env, info_node);
> + up_write(&env->bpf_progs.lock);
> +}
> +
> +void __perf_env__insert_bpf_prog_info(struct perf_env *env, struct bpf_prog_info_node *info_node)
> {
> __u32 prog_id = info_node->info_linear->info.id;
> struct bpf_prog_info_node *node;
> struct rb_node *parent = NULL;
> struct rb_node **p;
>
> - down_write(&env->bpf_progs.lock);
> p = &env->bpf_progs.infos.rb_node;
>
> while (*p != NULL) {
> @@ -42,15 +47,13 @@ void perf_env__insert_bpf_prog_info(struct perf_env *env,
> p = &(*p)->rb_right;
> } else {
> pr_debug("duplicated bpf prog info %u\n", prog_id);
> - goto out;
> + return;
> }
> }
>
> rb_link_node(&info_node->rb_node, parent, p);
> rb_insert_color(&info_node->rb_node, &env->bpf_progs.infos);
> env->bpf_progs.infos_cnt++;
> -out:
> - up_write(&env->bpf_progs.lock);
> }
>
> struct bpf_prog_info_node *perf_env__find_bpf_prog_info(struct perf_env *env,
> @@ -79,14 +82,22 @@ struct bpf_prog_info_node *perf_env__find_bpf_prog_info(struct perf_env *env,
> }
>
> bool perf_env__insert_btf(struct perf_env *env, struct btf_node *btf_node)
> +{
> + bool ret;
> +
> + down_write(&env->bpf_progs.lock);
> + ret = __perf_env__insert_btf(env, btf_node);
> + up_write(&env->bpf_progs.lock);
> + return ret;
> +}
> +
> +bool __perf_env__insert_btf(struct perf_env *env, struct btf_node *btf_node)
> {
> struct rb_node *parent = NULL;
> __u32 btf_id = btf_node->id;
> struct btf_node *node;
> struct rb_node **p;
> - bool ret = true;
>
> - down_write(&env->bpf_progs.lock);
> p = &env->bpf_progs.btfs.rb_node;
>
> while (*p != NULL) {
> @@ -98,25 +109,31 @@ bool perf_env__insert_btf(struct perf_env *env, struct btf_node *btf_node)
> p = &(*p)->rb_right;
> } else {
> pr_debug("duplicated btf %u\n", btf_id);
> - ret = false;
> - goto out;
> + return false;
> }
> }
>
> rb_link_node(&btf_node->rb_node, parent, p);
> rb_insert_color(&btf_node->rb_node, &env->bpf_progs.btfs);
> env->bpf_progs.btfs_cnt++;
> -out:
> - up_write(&env->bpf_progs.lock);
> - return ret;
> + return true;
> }
>
> struct btf_node *perf_env__find_btf(struct perf_env *env, __u32 btf_id)
> +{
> + struct btf_node *res;
> +
> + down_read(&env->bpf_progs.lock);
> + res = __perf_env__find_btf(env, btf_id);
> + up_read(&env->bpf_progs.lock);
> + return res;
> +}
> +
> +struct btf_node *__perf_env__find_btf(struct perf_env *env, __u32 btf_id)
> {
> struct btf_node *node = NULL;
> struct rb_node *n;
>
> - down_read(&env->bpf_progs.lock);
> n = env->bpf_progs.btfs.rb_node;
>
> while (n) {
> @@ -126,13 +143,9 @@ struct btf_node *perf_env__find_btf(struct perf_env *env, __u32 btf_id)
> else if (btf_id > node->id)
> n = n->rb_right;
> else
> - goto out;
> + return node;
> }
> - node = NULL;
> -
> -out:
> - up_read(&env->bpf_progs.lock);
> - return node;
> + return NULL;
> }
>
> /* purge data in bpf_progs.infos tree */
> diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
> index bf7e3c4c211f..7c527e65c186 100644
> --- a/tools/perf/util/env.h
> +++ b/tools/perf/util/env.h
> @@ -175,12 +175,16 @@ const char *perf_env__raw_arch(struct perf_env *env);
> int perf_env__nr_cpus_avail(struct perf_env *env);
>
> void perf_env__init(struct perf_env *env);
> +void __perf_env__insert_bpf_prog_info(struct perf_env *env,
> + struct bpf_prog_info_node *info_node);
> void perf_env__insert_bpf_prog_info(struct perf_env *env,
> struct bpf_prog_info_node *info_node);
> struct bpf_prog_info_node *perf_env__find_bpf_prog_info(struct perf_env *env,
> __u32 prog_id);
> bool perf_env__insert_btf(struct perf_env *env, struct btf_node *btf_node);
> +bool __perf_env__insert_btf(struct perf_env *env, struct btf_node *btf_node);
> struct btf_node *perf_env__find_btf(struct perf_env *env, __u32 btf_id);
> +struct btf_node *__perf_env__find_btf(struct perf_env *env, __u32 btf_id);
>
> int perf_env__numa_node(struct perf_env *env, struct perf_cpu cpu);
> char *perf_env__find_pmu_cap(struct perf_env *env, const char *pmu_name,
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 08cc2febabde..02bf9d8b5f74 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1849,8 +1849,8 @@ static void print_bpf_prog_info(struct feat_fd *ff, FILE *fp)
> node = rb_entry(next, struct bpf_prog_info_node, rb_node);
> next = rb_next(&node->rb_node);
>
> - bpf_event__print_bpf_prog_info(&node->info_linear->info,
> - env, fp);
> + __bpf_event__print_bpf_prog_info(&node->info_linear->info,
> + env, fp);
> }
>
> up_read(&env->bpf_progs.lock);
> @@ -3188,7 +3188,7 @@ static int process_bpf_prog_info(struct feat_fd *ff, void *data __maybe_unused)
> /* after reading from file, translate offset to address */
> bpil_offs_to_addr(info_linear);
> info_node->info_linear = info_linear;
> - perf_env__insert_bpf_prog_info(env, info_node);
> + __perf_env__insert_bpf_prog_info(env, info_node);
> }
>
> up_write(&env->bpf_progs.lock);
> @@ -3235,7 +3235,7 @@ static int process_bpf_btf(struct feat_fd *ff, void *data __maybe_unused)
> if (__do_read(ff, node->data, data_size))
> goto out;
>
> - perf_env__insert_btf(env, node);
> + __perf_env__insert_btf(env, node);
> node = NULL;
> }
>
> --
> 2.43.0.rc2.451.g8631bc7472-goog
>
next prev parent reply other threads:[~2024-01-03 9:26 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-07 1:46 [PATCH v1] perf env: Avoid recursively taking env->bpf_progs.lock Ian Rogers
2024-01-03 3:00 ` Ian Rogers
2024-01-03 16:27 ` Arnaldo Carvalho de Melo
2024-01-03 17:40 ` Song Liu
2024-01-03 20:55 ` Arnaldo Carvalho de Melo
2024-01-03 9:25 ` Jiri Olsa [this message]
2024-01-03 16:09 ` Arnaldo Carvalho de Melo
2024-01-03 16:24 ` Ian Rogers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZZUoJfGbPwLcFCnb@krava \
--to=olsajiri@gmail.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=bpf@vger.kernel.org \
--cc=chenhuacai@kernel.org \
--cc=irogers@google.com \
--cc=kan.liang@linux.intel.com \
--cc=kprateek.nayak@amd.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=ravi.bangoria@amd.com \
--cc=song@kernel.org \
--cc=wangming01@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.