| Message ID | 20231207014655.1252484-1-irogers@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v1] perf env: Avoid recursively taking env->bpf_progs.lock | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Dec 6, 2023 at 5:46 PM Ian Rogers <irogers@google.com> 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> Ping. Thanks, Ian > --- > 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(-) > > 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 >
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 >
Em Wed, Dec 06, 2023 at 05:46:55PM -0800, Ian Rogers escreveu: > 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(-) > > 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); > +} Minor nit/modification: diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c index cfdacbf294566c33..a459374d0a1a1dc8 100644 --- a/tools/perf/util/env.c +++ b/tools/perf/util/env.c @@ -22,7 +22,8 @@ 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); Just to reduce patch size a bit, just like happened with perf_env__insert_btf() further down, where its body was changed to call the __ variant but the non __ function got just the body modified. - Arnaldo > +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 >
On Wed, Jan 3, 2024 at 8:09 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Wed, Dec 06, 2023 at 05:46:55PM -0800, Ian Rogers escreveu: > > 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(-) > > > > 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); > > +} > > Minor nit/modification: > > diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c > index cfdacbf294566c33..a459374d0a1a1dc8 100644 > --- a/tools/perf/util/env.c > +++ b/tools/perf/util/env.c > @@ -22,7 +22,8 @@ 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); > > Just to reduce patch size a bit, just like happened with > perf_env__insert_btf() further down, where its body was changed to call > the __ variant but the non __ function got just the body modified. > > - Arnaldo Sorry for that and thanks for the fix. Ian > > +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 > > > > -- > > - Arnaldo
Em Tue, Jan 02, 2024 at 07:00:53PM -0800, Ian Rogers escreveu: > On Wed, Dec 6, 2023 at 5:46 PM Ian Rogers <irogers@google.com> 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> > > Ping. Applied, with that minor patch reduction hunk and this: Fixes: f8dfeae009effc0b ("perf bpf: Show more BPF program info in print_bpf_prog_info()") Song, can I have your Acked-by? - Arnaldo > > > --- > > 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(-) > > > > 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 > > >
On Wed, Jan 3, 2024 at 8:27 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Tue, Jan 02, 2024 at 07:00:53PM -0800, Ian Rogers escreveu: > > On Wed, Dec 6, 2023 at 5:46 PM Ian Rogers <irogers@google.com> 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> > > > > Ping. > > Applied, with that minor patch reduction hunk and this: > > Fixes: f8dfeae009effc0b ("perf bpf: Show more BPF program info in print_bpf_prog_info()") > > Song, can I have your Acked-by? LGTM. Thanks for the fix! Acked-by: Song Liu <song@kernel.org>
Em Wed, Jan 03, 2024 at 09:40:26AM -0800, Song Liu escreveu: > On Wed, Jan 3, 2024 at 8:27 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Applied, with that minor patch reduction hunk and this: > > Fixes: f8dfeae009effc0b ("perf bpf: Show more BPF program info in print_bpf_prog_info()") > > Song, can I have your Acked-by? > LGTM. Thanks for the fix! > Acked-by: Song Liu <song@kernel.org> Thanks! Added to the cset. - Arnaldo
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; }
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(-)