| 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: Wed, 6 Dec 2023 15:39:06 -0800
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
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, Dec 4, 2023 at 3:59 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> 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.
Agreed on the C++ issue, but this is also an issue in linux/list.h
that uses the same naming convention. The reason for the change from
map to new is that "pos->map" and "map" read very similarly, so the
code is more intention revealing with the change.
> > +{
> > +
> > + 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();
Good idea. Done in v6.
Thanks,
Ian
> 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