[v5,18/50] perf maps: Refactor maps__fixup_overlappings
Commit Message
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@google.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(-)
Comments
On Mon, Nov 27, 2023 at 2:10 PM Ian Rogers <irogers@google.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@google.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
>
On Mon, Dec 4, 2023 at 3:59 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Nov 27, 2023 at 2:10 PM Ian Rogers <irogers@google.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@google.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
> >
@@ -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.
+ * 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)
+{
+
+ struct rb_node *next;
+ int err = 0;
+
+ 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;
}
@@ -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);
@@ -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 {