From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: "Peter Zijlstra" <peterz@infradead.org>,
"Ingo Molnar" <mingo@redhat.com>,
"Mark Rutland" <mark.rutland@arm.com>,
"Alexander Shishkin" <alexander.shishkin@linux.intel.com>,
"Jiri Olsa" <jolsa@kernel.org>,
"Namhyung Kim" <namhyung@kernel.org>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Darren Hart" <dvhart@infradead.org>,
"Davidlohr Bueso" <dave@stgolabs.net>,
"André Almeida" <andrealmeid@collabora.com>,
"James Clark" <james.clark@arm.com>,
"John Garry" <john.g.garry@oracle.com>,
"Riccardo Mancini" <rickyman7@gmail.com>,
"Yury Norov" <yury.norov@gmail.com>,
"Andy Shevchenko" <andriy.shevchenko@linux.intel.com>,
"Andrew Morton" <akpm@linux-foundation.org>,
"Adrian Hunter" <adrian.hunter@intel.com>,
"Leo Yan" <leo.yan@linaro.org>, "Andi Kleen" <ak@linux.intel.com>,
"Thomas Richter" <tmricht@linux.ibm.com>,
"Kan Liang" <kan.liang@linux.intel.com>,
"Madhavan Srinivasan" <maddy@linux.ibm.com>,
"Shunsuke Nakamura" <nakamura.shun@fujitsu.com>,
"Song Liu" <song@kernel.org>,
"Masami Hiramatsu" <mhiramat@kernel.org>,
"Steven Rostedt" <rostedt@goodmis.org>,
"Miaoqian Lin" <linmq006@gmail.com>,
"Stephen Brennan" <stephen.s.brennan@oracle.com>,
"Kajol Jain" <kjain@linux.ibm.com>,
"Alexey Bayduraev" <alexey.v.bayduraev@linux.intel.com>,
"German Gomez" <german.gomez@arm.com>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
"Eric Dumazet" <edumazet@google.com>,
"Dmitry Vyukov" <dvyukov@google.com>,
"Hao Luo" <haoluo@google.com>,
"Stephane Eranian" <eranian@google.com>
Subject: Re: [PATCH v4 06/22] perf map: Move map list node into symbol
Date: Mon, 20 Mar 2023 12:48:36 -0300 [thread overview]
Message-ID: <ZBiAVI6q36shEKF4@kernel.org> (raw)
In-Reply-To: <20230320033810.980165-7-irogers@google.com>
Em Sun, Mar 19, 2023 at 08:37:54PM -0700, Ian Rogers escreveu:
> Using a perf map as a list node is only done in symbol. Move the
> list_node struct into symbol as a single pointer to the map. This
> makes reference count behavior more obvious and easy to check.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/map.h | 5 +--
> tools/perf/util/symbol.c | 90 ++++++++++++++++++++++++++--------------
> 2 files changed, 60 insertions(+), 35 deletions(-)
>
> diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
> index 3dcfe06db6b3..2879cae05ee0 100644
> --- a/tools/perf/util/map.h
> +++ b/tools/perf/util/map.h
> @@ -16,10 +16,7 @@ struct maps;
> struct machine;
>
> struct map {
> - union {
> - struct rb_node rb_node;
> - struct list_head node;
> - };
> + struct rb_node rb_node;
> u64 start;
> u64 end;
> bool erange_warned:1;
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index a458aa8b87bb..2676a163e237 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -48,6 +48,11 @@ static bool symbol__is_idle(const char *name);
> int vmlinux_path__nr_entries;
> char **vmlinux_path;
>
> +struct map_list_node {
> + struct list_head node;
> + struct map *map;
> +};
> +
> struct symbol_conf symbol_conf = {
> .nanosecs = false,
> .use_modules = true,
> @@ -1219,16 +1224,22 @@ struct kcore_mapfn_data {
> static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data)
> {
> struct kcore_mapfn_data *md = data;
> - struct map *map;
> + struct map_list_node *list_node;
> +
> + list_node = malloc(sizeof(*list_node));
> + if (list_node == NULL)
> + return -ENOMEM;
So this is open coding map_list_node__new(), can we have it defined
right after the struct map_list_node definition?
I've applied the 1-5 patches in this series to my local branch, testing
now.
- Arnaldo
>
> - map = map__new2(start, md->dso);
> - if (map == NULL)
> + list_node->map = map__new2(start, md->dso);
> + if (list_node->map == NULL) {
> + free(list_node);
> return -ENOMEM;
> + }
>
> - map->end = map->start + len;
> - map->pgoff = pgoff;
> + list_node->map->end = list_node->map->start + len;
> + list_node->map->pgoff = pgoff;
>
> - list_add(&map->node, &md->maps);
> + list_add(&list_node->node, &md->maps);
>
> return 0;
> }
> @@ -1264,12 +1275,19 @@ int maps__merge_in(struct maps *kmaps, struct map *new_map)
> * |new.............| -> |new..| |new..|
> * |old....| -> |old....|
> */
> - struct map *m = map__clone(new_map);
> + struct map_list_node *m;
>
> + m = malloc(sizeof(*m));
> if (!m)
> return -ENOMEM;
>
> - m->end = old_map->start;
> + m->map = map__clone(new_map);
> + if (!m->map) {
> + free(m);
> + return -ENOMEM;
> + }
> +
> + m->map->end = old_map->start;
> list_add_tail(&m->node, &merged);
> new_map->pgoff += old_map->end - new_map->start;
> new_map->start = old_map->end;
> @@ -1299,10 +1317,13 @@ int maps__merge_in(struct maps *kmaps, struct map *new_map)
> }
>
> while (!list_empty(&merged)) {
> - old_map = list_entry(merged.next, struct map, node);
> - list_del_init(&old_map->node);
> - maps__insert(kmaps, old_map);
> - map__put(old_map);
> + struct map_list_node *old_node;
> +
> + old_node = list_entry(merged.next, struct map_list_node, node);
> + list_del_init(&old_node->node);
> + maps__insert(kmaps, old_node->map);
> + map__put(old_node->map);
> + free(old_node);
> }
>
> if (new_map) {
> @@ -1317,7 +1338,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
> {
> struct maps *kmaps = map__kmaps(map);
> struct kcore_mapfn_data md;
> - struct map *old_map, *new_map, *replacement_map = NULL, *next;
> + struct map *old_map, *replacement_map = NULL, *next;
> struct machine *machine;
> bool is_64_bit;
> int err, fd;
> @@ -1378,11 +1399,12 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
> /* Find the kernel map using the '_stext' symbol */
> if (!kallsyms__get_function_start(kallsyms_filename, "_stext", &stext)) {
> u64 replacement_size = 0;
> + struct map_list_node *new_node;
>
> - list_for_each_entry(new_map, &md.maps, node) {
> - u64 new_size = new_map->end - new_map->start;
> + list_for_each_entry(new_node, &md.maps, node) {
> + u64 new_size = new_node->map->end - new_node->map->start;
>
> - if (!(stext >= new_map->start && stext < new_map->end))
> + if (!(stext >= new_node->map->start && stext < new_node->map->end))
> continue;
>
> /*
> @@ -1392,40 +1414,43 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
> * falls within more than one in the list.
> */
> if (!replacement_map || new_size < replacement_size) {
> - replacement_map = new_map;
> + replacement_map = new_node->map;
> replacement_size = new_size;
> }
> }
> }
>
> if (!replacement_map)
> - replacement_map = list_entry(md.maps.next, struct map, node);
> + replacement_map = list_entry(md.maps.next, struct map_list_node, node)->map;
>
> /* Add new maps */
> while (!list_empty(&md.maps)) {
> - new_map = list_entry(md.maps.next, struct map, node);
> - list_del_init(&new_map->node);
> - if (new_map == replacement_map) {
> - map->start = new_map->start;
> - map->end = new_map->end;
> - map->pgoff = new_map->pgoff;
> - map->map_ip = new_map->map_ip;
> - map->unmap_ip = new_map->unmap_ip;
> + struct map_list_node *new_node;
> +
> + new_node = list_entry(md.maps.next, struct map_list_node, node);
> + list_del_init(&new_node->node);
> + if (new_node->map == replacement_map) {
> + map->start = new_node->map->start;
> + map->end = new_node->map->end;
> + map->pgoff = new_node->map->pgoff;
> + map->map_ip = new_node->map->map_ip;
> + map->unmap_ip = new_node->map->unmap_ip;
> /* Ensure maps are correctly ordered */
> map__get(map);
> maps__remove(kmaps, map);
> maps__insert(kmaps, map);
> map__put(map);
> - map__put(new_map);
> + map__put(new_node->map);
> } else {
> /*
> * Merge kcore map into existing maps,
> * and ensure that current maps (eBPF)
> * stay intact.
> */
> - if (maps__merge_in(kmaps, new_map))
> + if (maps__merge_in(kmaps, new_node->map))
> goto out_err;
> }
> + free(new_node);
> }
>
> if (machine__is(machine, "x86_64")) {
> @@ -1462,9 +1487,12 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>
> out_err:
> while (!list_empty(&md.maps)) {
> - map = list_entry(md.maps.next, struct map, node);
> - list_del_init(&map->node);
> - map__put(map);
> + struct map_list_node *list_node;
> +
> + list_node = list_entry(md.maps.next, struct map_list_node, node);
> + list_del_init(&list_node->node);
> + map__put(list_node->map);
> + free(list_node);
> }
> close(fd);
> return -EINVAL;
> --
> 2.40.0.rc1.284.g88254d51c5-goog
>
--
- Arnaldo
next prev parent reply other threads:[~2023-03-20 15:57 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-20 3:37 [PATCH v4 00/22] Reference count checker and related fixes Ian Rogers
2023-03-20 3:37 ` [PATCH v4 01/22] perf symbol: Avoid memory leak from abi::__cxa_demangle Ian Rogers
2023-03-20 3:37 ` [PATCH v4 02/22] perf bpf_counter: Use public cpumap accessors Ian Rogers
2023-03-20 3:37 ` [PATCH v4 03/22] perf tests: Add common error route for code-reading Ian Rogers
2023-03-20 3:37 ` [PATCH v4 04/22] perf test: Fix memory leak in symbols Ian Rogers
2023-03-20 8:06 ` Adrian Hunter
2023-03-20 13:04 ` Ian Rogers
2023-03-20 3:37 ` [PATCH v4 05/22] perf symbol: Sort names under write lock Ian Rogers
2023-03-20 3:37 ` [PATCH v4 06/22] perf map: Move map list node into symbol Ian Rogers
2023-03-20 15:48 ` Arnaldo Carvalho de Melo [this message]
2023-03-20 16:51 ` Ian Rogers
2023-03-20 3:37 ` [PATCH v4 07/22] perf maps: Remove rb_node from struct map Ian Rogers
2023-03-20 3:37 ` [PATCH v4 08/22] perf maps: Add functions to access maps Ian Rogers
2023-03-20 3:37 ` [PATCH v4 09/22] perf map: Add accessor for dso Ian Rogers
2023-03-20 3:37 ` [PATCH v4 10/22] perf map: Add accessor for start and end Ian Rogers
2023-03-20 3:37 ` [PATCH v4 11/22] perf map: Rename map_ip and unmap_ip Ian Rogers
2023-03-20 3:38 ` [PATCH v4 12/22] perf map: Add helper for " Ian Rogers
2023-03-20 3:38 ` [PATCH v4 13/22] perf map: Add accessors for prot, priv and flags Ian Rogers
2023-03-20 3:38 ` [PATCH v4 14/22] perf map: Add accessors for pgoff and reloc Ian Rogers
2023-03-20 3:38 ` [PATCH v4 15/22] perf test: Add extra diagnostics to maps test Ian Rogers
2023-03-20 3:38 ` [PATCH v4 16/22] perf maps: Modify maps_by_name to hold a reference to a map Ian Rogers
2023-03-20 3:38 ` [PATCH v4 17/22] perf map: Changes to reference counting Ian Rogers
2023-03-20 11:18 ` Adrian Hunter
2023-03-20 17:00 ` Ian Rogers
2023-03-20 3:38 ` [PATCH v4 18/22] libperf: Add reference count checking macros Ian Rogers
2023-03-20 3:38 ` [PATCH v4 19/22] perf cpumap: Add reference count checking Ian Rogers
2023-03-20 3:38 ` [PATCH v4 20/22] perf namespaces: " Ian Rogers
2023-03-20 3:38 ` [PATCH v4 21/22] perf maps: " Ian Rogers
2023-03-20 3:38 ` [PATCH v4 22/22] perf map: " 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=ZBiAVI6q36shEKF4@kernel.org \
--to=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=ak@linux.intel.com \
--cc=akpm@linux-foundation.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=alexey.v.bayduraev@linux.intel.com \
--cc=andrealmeid@collabora.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=dave@stgolabs.net \
--cc=dvhart@infradead.org \
--cc=dvyukov@google.com \
--cc=edumazet@google.com \
--cc=eranian@google.com \
--cc=german.gomez@arm.com \
--cc=haoluo@google.com \
--cc=irogers@google.com \
--cc=james.clark@arm.com \
--cc=john.g.garry@oracle.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=kjain@linux.ibm.com \
--cc=leo.yan@linaro.org \
--cc=linmq006@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=maddy@linux.ibm.com \
--cc=mark.rutland@arm.com \
--cc=mhiramat@kernel.org \
--cc=mingo@redhat.com \
--cc=nakamura.shun@fujitsu.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=rickyman7@gmail.com \
--cc=rostedt@goodmis.org \
--cc=song@kernel.org \
--cc=stephen.s.brennan@oracle.com \
--cc=tglx@linutronix.de \
--cc=tmricht@linux.ibm.com \
--cc=yury.norov@gmail.com \
/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.