Re: [PATCH v5 18/50] perf maps: Refactor maps__fixup_overlappings

From: Ian Rogers
Date: Wed Dec 06 2023 - 18:43:06 EST

Next message: Conor Dooley: "Re: [PATCH 6.1 000/105] 6.1.66-rc2 review"
Previous message: Stephen Rothwell: "linux-next: manual merge of the vfs-brauner tree with the btrfs tree"
In reply to: Namhyung Kim: "Re: [PATCH v5 18/50] perf maps: Refactor maps__fixup_overlappings"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Dec 4, 2023 at 3:59 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Mon, Nov 27, 2023 at 2:10 PM Ian Rogers <irogers@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
> >

Next message: Conor Dooley: "Re: [PATCH 6.1 000/105] 6.1.66-rc2 review"
Previous message: Stephen Rothwell: "linux-next: manual merge of the vfs-brauner tree with the btrfs tree"
In reply to: Namhyung Kim: "Re: [PATCH v5 18/50] perf maps: Refactor maps__fixup_overlappings"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]