| 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
| ||
|
Message-ID: <CAM9d7chbysNaH++LQgeskZ_LRAF7KuErexapDTWRf-zsgPOq1g@mail.gmail.com>
Date: Mon, 4 Dec 2023 15:59:09 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Nick Terrell <terrelln@...com>,
Kan Liang <kan.liang@...ux.intel.com>,
Andi Kleen <ak@...ux.intel.com>,
Kajol Jain <kjain@...ux.ibm.com>,
Athira Rajeev <atrajeev@...ux.vnet.ibm.com>,
Huacai Chen <chenhuacai@...nel.org>,
Masami Hiramatsu <mhiramat@...nel.org>,
Vincent Whitchurch <vincent.whitchurch@...s.com>,
"Steinar H. Gunderson" <sesse@...gle.com>,
Liam Howlett <liam.howlett@...cle.com>,
Miguel Ojeda <ojeda@...nel.org>,
Colin Ian King <colin.i.king@...il.com>,
Dmitrii Dolgov <9erthalion6@...il.com>,
Yang Jihong <yangjihong1@...wei.com>,
Ming Wang <wangming01@...ngson.cn>,
James Clark <james.clark@....com>,
K Prateek Nayak <kprateek.nayak@....com>,
Sean Christopherson <seanjc@...gle.com>,
Leo Yan <leo.yan@...aro.org>,
Ravi Bangoria <ravi.bangoria@....com>,
German Gomez <german.gomez@....com>,
Changbin Du <changbin.du@...wei.com>,
Paolo Bonzini <pbonzini@...hat.com>, Li Dong <lidong@...o.com>,
Sandipan Das <sandipan.das@....com>,
liuwenyu <liuwenyu7@...wei.com>, linux-kernel@...r.kernel.org,
linux-perf-users@...r.kernel.org,
Guilherme Amadio <amadio@...too.org>
Subject: Re: [PATCH v5 18/50] perf maps: Refactor maps__fixup_overlappings
On Mon, Nov 27, 2023 at 2:10 PM Ian Rogers <irogers@...gle.com> wrote:
>
> Rename to maps__fixup_overlap_and_insert as the given mapping is
> always inserted. Factor out first_ending_after as a utility
> function. Minor variable name changes. Switch to using debug_file()
> rather than passing a debug FILE*.
>
> Signed-off-by: Ian Rogers <irogers@...gle.com>
> ---
> tools/perf/util/maps.c | 62 ++++++++++++++++++++++++----------------
> tools/perf/util/maps.h | 2 +-
> tools/perf/util/thread.c | 3 +-
> 3 files changed, 39 insertions(+), 28 deletions(-)
>
> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index f13fd3a9686b..40df08dd9bf3 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
> @@ -334,20 +334,16 @@ size_t maps__fprintf(struct maps *maps, FILE *fp)
> return args.printed;
> }
>
> -int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp)
> +/*
> + * Find first map where end > new->start.
s/new/map/.
> + * Same as find_vma() in kernel.
> + */
> +static struct rb_node *first_ending_after(struct maps *maps, const struct map *map)
> {
> struct rb_root *root;
> struct rb_node *next, *first;
> - int err = 0;
> -
> - down_write(maps__lock(maps));
>
> root = maps__entries(maps);
> -
> - /*
> - * Find first map where end > map->start.
> - * Same as find_vma() in kernel.
> - */
> next = root->rb_node;
> first = NULL;
> while (next) {
> @@ -361,8 +357,22 @@ int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp)
> } else
> next = next->rb_right;
> }
> + return first;
> +}
> +
> +/*
> + * Adds new to maps, if new overlaps existing entries then the existing maps are
> + * adjusted or removed so that new fits without overlapping any entries.
> + */
> +int maps__fixup_overlap_and_insert(struct maps *maps, struct map *new)
Do we really need this rename (map -> new)? It seems just to create
unnecessary diffs. Also I'd like to avoid 'new' as it's a keyword in C++
although we don't compile with C++ compilers.
> +{
> +
> + struct rb_node *next;
> + int err = 0;
Maybe you can add this line or let the caller pass it to reduce the diff.
FILE *fp = debug_file();
Thanks,
Namhyung
> +
> + down_write(maps__lock(maps));
>
> - next = first;
> + next = first_ending_after(maps, new);
> while (next && !err) {
> struct map_rb_node *pos = rb_entry(next, struct map_rb_node, rb_node);
> next = rb_next(&pos->rb_node);
> @@ -371,27 +381,27 @@ int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp)
> * Stop if current map starts after map->end.
> * Maps are ordered by start: next will not overlap for sure.
> */
> - if (map__start(pos->map) >= map__end(map))
> + if (map__start(pos->map) >= map__end(new))
> break;
>
> if (verbose >= 2) {
>
> if (use_browser) {
> pr_debug("overlapping maps in %s (disable tui for more info)\n",
> - map__dso(map)->name);
> + map__dso(new)->name);
> } else {
> - fputs("overlapping maps:\n", fp);
> - map__fprintf(map, fp);
> - map__fprintf(pos->map, fp);
> + pr_debug("overlapping maps:\n");
> + map__fprintf(new, debug_file());
> + map__fprintf(pos->map, debug_file());
> }
> }
>
> - rb_erase_init(&pos->rb_node, root);
> + rb_erase_init(&pos->rb_node, maps__entries(maps));
> /*
> * Now check if we need to create new maps for areas not
> * overlapped by the new map:
> */
> - if (map__start(map) > map__start(pos->map)) {
> + if (map__start(new) > map__start(pos->map)) {
> struct map *before = map__clone(pos->map);
>
> if (before == NULL) {
> @@ -399,7 +409,7 @@ int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp)
> goto put_map;
> }
>
> - map__set_end(before, map__start(map));
> + map__set_end(before, map__start(new));
> err = __maps__insert(maps, before);
> if (err) {
> map__put(before);
> @@ -407,11 +417,11 @@ int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp)
> }
>
> if (verbose >= 2 && !use_browser)
> - map__fprintf(before, fp);
> + map__fprintf(before, debug_file());
> map__put(before);
> }
>
> - if (map__end(map) < map__end(pos->map)) {
> + if (map__end(new) < map__end(pos->map)) {
> struct map *after = map__clone(pos->map);
>
> if (after == NULL) {
> @@ -419,23 +429,25 @@ int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp)
> goto put_map;
> }
>
> - map__set_start(after, map__end(map));
> - map__add_pgoff(after, map__end(map) - map__start(pos->map));
> - assert(map__map_ip(pos->map, map__end(map)) ==
> - map__map_ip(after, map__end(map)));
> + map__set_start(after, map__end(new));
> + map__add_pgoff(after, map__end(new) - map__start(pos->map));
> + assert(map__map_ip(pos->map, map__end(new)) ==
> + map__map_ip(after, map__end(new)));
> err = __maps__insert(maps, after);
> if (err) {
> map__put(after);
> goto put_map;
> }
> if (verbose >= 2 && !use_browser)
> - map__fprintf(after, fp);
> + map__fprintf(after, debug_file());
> map__put(after);
> }
> put_map:
> map__put(pos->map);
> free(pos);
> }
> + /* Add the map. */
> + err = __maps__insert(maps, new);
> up_write(maps__lock(maps));
> return err;
> }
> diff --git a/tools/perf/util/maps.h b/tools/perf/util/maps.h
> index b94ad5c8fea7..62e94d443c02 100644
> --- a/tools/perf/util/maps.h
> +++ b/tools/perf/util/maps.h
> @@ -133,7 +133,7 @@ struct addr_map_symbol;
>
> int maps__find_ams(struct maps *maps, struct addr_map_symbol *ams);
>
> -int maps__fixup_overlappings(struct maps *maps, struct map *map, FILE *fp);
> +int maps__fixup_overlap_and_insert(struct maps *maps, struct map *new);
>
> struct map *maps__find_by_name(struct maps *maps, const char *name);
>
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index b6986a81aa6d..3d47b5c5528b 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -345,8 +345,7 @@ int thread__insert_map(struct thread *thread, struct map *map)
> if (ret)
> return ret;
>
> - maps__fixup_overlappings(thread__maps(thread), map, stderr);
> - return maps__insert(thread__maps(thread), map);
> + return maps__fixup_overlap_and_insert(thread__maps(thread), map);
> }
>
> struct thread__prepare_access_maps_cb_args {
> --
> 2.43.0.rc1.413.gea7ed67945-goog
>
Powered by blists - more mailing lists