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

[Namhyung Kim]

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.

> +{
> +
> +       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
>