[PATCH 0/6] Fixups for kernel maps insertion

[PATCH 0/6] Fixups for kernel maps insertion

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Hi Namhyung,

	Here are the patches from the recent session on fixing the build
when DEBUG=1 is used, please take a look.

	I think that since there are no reported problems and ended up
being a sizeable series, we better apply it to perf-tools-next,

Best Regards,

- Arnaldo

Arnaldo Carvalho de Melo (3):
  perf maps: Introduce map__set_kmap() for kernel maps
  perf maps: Set the kmaps for newly created/added kernel maps
  perf maps: Add missing map__set_kmap() when replacing a kernel map

Namhyung Kim (3):
  perf machine: Fixup kernel maps ends after adding extra maps
  perf maps: Fixup maps_by_name when modifying maps_by_address
  perf machine: Fix insertion of PERF_RECORD_KSYMBOL related kernel maps

 tools/perf/util/machine.c |  6 ++--
 tools/perf/util/maps.c    | 58 +++++++++++++++++++++++++++++++++------
 2 files changed, 52 insertions(+), 12 deletions(-)

-- 
2.47.0

[PATCH 1/6] perf maps: Introduce map__set_kmap() for kernel maps

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

We need to set it in other places than __maps__insert(), so that we can
have access to the 'struct kmap' from a kernel 'struct map'.

When building perf with 'DEBUG=1' we can notice it failing a consistency
check done in the check_invariants() function:

  root@number:~# perf record -- perf test -w offcpu
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.040 MB perf.data (23 samples) ]
  perf: util/maps.c:95: check_invariants: Assertion `map__end(prev) <= map__end(map)' failed.
  Aborted (core dumped)
  root@number:~#

The investigation on that was happening bisected to 876e80cf83d10585
("perf tools: Fixup end address of modules"), and the following patches
will plug the problems found, this patch is just legwork on that
direction.

Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: https://lore.kernel.org/lkml/Z74V0hZXrTLM6VIJ@x1
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/maps.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
index 09c9cc326c08d435..e21d29f5df01c6f7 100644
--- a/tools/perf/util/maps.c
+++ b/tools/perf/util/maps.c
@@ -428,11 +428,29 @@ static unsigned int maps__by_name_index(const struct maps *maps, const struct ma
 	return -1;
 }
 
+static void map__set_kmap(struct map *map, struct maps *maps)
+{
+	struct dso *dso;
+
+	if (map == NULL)
+		return;
+
+	dso = map__dso(map);
+
+	if (dso && dso__kernel(dso)) {
+                struct kmap *kmap = map__kmap(map);
+
+                if (kmap)
+                        kmap->kmaps = maps;
+                else
+                        pr_err("Internal error: kernel dso with non kernel map\n");
+        }
+}
+
 static int __maps__insert(struct maps *maps, struct map *new)
 {
 	struct map **maps_by_address = maps__maps_by_address(maps);
 	struct map **maps_by_name = maps__maps_by_name(maps);
-	const struct dso *dso = map__dso(new);
 	unsigned int nr_maps = maps__nr_maps(maps);
 	unsigned int nr_allocate = RC_CHK_ACCESS(maps)->nr_maps_allocated;
 
@@ -483,14 +501,9 @@ static int __maps__insert(struct maps *maps, struct map *new)
 	}
 	if (map__end(new) < map__start(new))
 		RC_CHK_ACCESS(maps)->ends_broken = true;
-	if (dso && dso__kernel(dso)) {
-		struct kmap *kmap = map__kmap(new);
 
-		if (kmap)
-			kmap->kmaps = maps;
-		else
-			pr_err("Internal error: kernel dso with non kernel map\n");
-	}
+	map__set_kmap(new, maps);
+
 	return 0;
 }
 
-- 
2.47.0

[PATCH 2/6] perf maps: Set the kmaps for newly created/added kernel maps

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

When using __maps__insert_sorted() the map kmaps field needs to be
initialized, as we need kernel maps to work with map__kmap().

Fix it by using the newly introduced map__set_kmap() method.

Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: https://lore.kernel.org/lkml/Z74V0hZXrTLM6VIJ@x1
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/maps.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
index e21d29f5df01c6f7..dec2e04696c9097e 100644
--- a/tools/perf/util/maps.c
+++ b/tools/perf/util/maps.c
@@ -798,6 +798,9 @@ static int __maps__insert_sorted(struct maps *maps, unsigned int first_after_ind
 	}
 	RC_CHK_ACCESS(maps)->nr_maps = nr_maps + to_add;
 	maps__set_maps_by_name_sorted(maps, false);
+	map__set_kmap(new1, maps);
+	map__set_kmap(new2, maps);
+
 	check_invariants(maps);
 	return 0;
 }
-- 
2.47.0

[PATCH 3/6] perf machine: Fixup kernel maps ends after adding extra maps

[Arnaldo Carvalho de Melo]

From: Namhyung Kim <namhyung@kernel.org>

I just noticed it would add extra kernel maps after modules.  I think it
should fixup end address of the kernel maps after adding all maps first.

Fixes: 876e80cf83d10585 ("perf tools: Fixup end address of modules")
Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: https://lore.kernel.org/lkml/Z7TvZGjVix2asYWI@x1
Link: https://lore.kernel.org/lkml/Z712hzvv22Ni63f1@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 3f1faf94198dbe56..f7df01adad61ceea 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1462,8 +1462,6 @@ static int machine__create_modules(struct machine *machine)
 	if (modules__parse(modules, machine, machine__create_module))
 		return -1;
 
-	maps__fixup_end(machine__kernel_maps(machine));
-
 	if (!machine__set_modules_path(machine))
 		return 0;
 
@@ -1557,6 +1555,8 @@ int machine__create_kernel_maps(struct machine *machine)
 		}
 	}
 
+	maps__fixup_end(machine__kernel_maps(machine));
+
 out_put:
 	dso__put(kernel);
 	return ret;
-- 
2.47.0

[PATCH 4/6] perf maps: Fixup maps_by_name when modifying maps_by_address

[Arnaldo Carvalho de Melo]

From: Namhyung Kim <namhyung@kernel.org>

We can't just replacing the map in the maps_by_address and not touching
on the maps_by_name, that would leave the refcount as 1 and thus trip
another consistency check, this one:

  perf: util/maps.c:110: check_invariants:
  	Assertion `refcount_read(map__refcnt(map)) > 1' failed.

  106         /*
  107          * Maps by name maps should be in maps_by_address, so
  108          * the reference count should be higher.
  109          */
  110         assert(refcount_read(map__refcnt(map)) > 1);

Committer notice:

Initialize the newly added 'ni' variable, that really can't be
accessed unitialized trips some gcc versions, like:

  12    20.00 archlinux:base                : FAIL gcc version 13.2.1 20230801 (GCC)
    util/maps.c: In function ‘__maps__fixup_overlap_and_insert’:
    util/maps.c:896:54: error: ‘ni’ may be used uninitialized [-Werror=maybe-uninitialized]
      896 |                                 map__put(maps_by_name[ni]);
          |                                                      ^
    util/maps.c:816:25: note: ‘ni’ was declared here
      816 |         unsigned int i, ni;
          |                         ^~
    cc1: all warnings being treated as errors
    make[3]: *** [/git/perf-6.14.0-rc1/tools/build/Makefile.build:138: util] Error 2

Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: https://lore.kernel.org/lkml/Z79std66tPq-nqsD@google.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/maps.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
index dec2e04696c9097e..dffc54a8a29bf3b0 100644
--- a/tools/perf/util/maps.c
+++ b/tools/perf/util/maps.c
@@ -813,7 +813,7 @@ static int __maps__fixup_overlap_and_insert(struct maps *maps, struct map *new)
 {
 	int err = 0;
 	FILE *fp = debug_file();
-	unsigned int i;
+	unsigned int i, ni = INT_MAX; // Some gcc complain, but depends on maps_by_name...
 
 	if (!maps__maps_by_address_sorted(maps))
 		__maps__sort_by_address(maps);
@@ -824,6 +824,7 @@ static int __maps__fixup_overlap_and_insert(struct maps *maps, struct map *new)
 	 */
 	for (i = first_ending_after(maps, new); i < maps__nr_maps(maps); ) {
 		struct map **maps_by_address = maps__maps_by_address(maps);
+		struct map **maps_by_name = maps__maps_by_name(maps);
 		struct map *pos = maps_by_address[i];
 		struct map *before = NULL, *after = NULL;
 
@@ -843,6 +844,9 @@ static int __maps__fixup_overlap_and_insert(struct maps *maps, struct map *new)
 			map__fprintf(pos, fp);
 		}
 
+		if (maps_by_name)
+			ni = maps__by_name_index(maps, pos);
+
 		/*
 		 * Now check if we need to create new maps for areas not
 		 * overlapped by the new map:
@@ -887,6 +891,12 @@ static int __maps__fixup_overlap_and_insert(struct maps *maps, struct map *new)
 		if (before) {
 			map__put(maps_by_address[i]);
 			maps_by_address[i] = before;
+
+			if (maps_by_name) {
+				map__put(maps_by_name[ni]);
+				maps_by_name[ni] = map__get(before);
+			}
+
 			/* Maps are still ordered, go to next one. */
 			i++;
 			if (after) {
@@ -908,6 +918,12 @@ static int __maps__fixup_overlap_and_insert(struct maps *maps, struct map *new)
 			 */
 			map__put(maps_by_address[i]);
 			maps_by_address[i] = map__get(new);
+
+			if (maps_by_name) {
+				map__put(maps_by_name[ni]);
+				maps_by_name[ni] = map__get(new);
+			}
+
 			err = __maps__insert_sorted(maps, i + 1, after, NULL);
 			map__put(after);
 			check_invariants(maps);
@@ -926,6 +942,12 @@ static int __maps__fixup_overlap_and_insert(struct maps *maps, struct map *new)
 				 */
 				map__put(maps_by_address[i]);
 				maps_by_address[i] = map__get(new);
+
+				if (maps_by_name) {
+					map__put(maps_by_name[ni]);
+					maps_by_name[ni] = map__get(new);
+				}
+
 				check_invariants(maps);
 				return err;
 			}
-- 
2.47.0

[PATCH 5/6] perf maps: Add missing map__set_kmap() when replacing a kernel map

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Since in this case __maps__insert_sorted() is not called and thus
doesn't have the opportunity to do the needed map__set_kmap() calls on
the new map.

Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: https://lore.kernel.org/lkml/Z7-May5w9VQd5QD0@x1
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/maps.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
index dffc54a8a29bf3b0..081466b3b4676044 100644
--- a/tools/perf/util/maps.c
+++ b/tools/perf/util/maps.c
@@ -948,6 +948,8 @@ static int __maps__fixup_overlap_and_insert(struct maps *maps, struct map *new)
 					maps_by_name[ni] = map__get(new);
 				}
 
+				map__set_kmap(new, maps);
+
 				check_invariants(maps);
 				return err;
 			}
-- 
2.47.0

[PATCH 6/6] perf machine: Fix insertion of PERF_RECORD_KSYMBOL related kernel maps

[Arnaldo Carvalho de Melo]

From: Namhyung Kim <namhyung@kernel.org>

This was detected at the end of a 'perf record' session when build-id
collection was enabled and thus the BPF programs put in place while the
session was running, some even put in place by perf itself were
processed and inserted, with some overlaps related to BPF trampolines
and programs took place.

Using maps__fixup_overlap_and_insert() instead of maps__insert() "fixes"
the problem, in the sense that overlaps will be dealt with and then the
consistency will be kept, but it would be interesting to fully
understand why such overlaps take place and how to deal with them when
doing symbol resolution.

Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Suggested-by: Ian Rogers <irogers@google.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: https://lore.kernel.org/lkml/CAP-5=fXEEMFgPF2aZhKsfrY_En+qoqX20dWfuE_ad73Uxf0ZHQ@mail.gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index f7df01adad61ceea..a81ffd2d1a054d60 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -718,7 +718,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
 
 		map__set_start(map, event->ksymbol.addr);
 		map__set_end(map, map__start(map) + event->ksymbol.len);
-		err = maps__insert(machine__kernel_maps(machine), map);
+		err = maps__fixup_overlap_and_insert(machine__kernel_maps(machine), map);
 		if (err) {
 			err = -ENOMEM;
 			goto out;
-- 
2.47.0

Re: [PATCH 1/6] perf maps: Introduce map__set_kmap() for kernel maps

[Ian Rogers]

On Thu, Feb 27, 2025 at 1:55 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> We need to set it in other places than __maps__insert(), so that we can
> have access to the 'struct kmap' from a kernel 'struct map'.
>
> When building perf with 'DEBUG=1' we can notice it failing a consistency
> check done in the check_invariants() function:
>
>   root@number:~# perf record -- perf test -w offcpu
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.040 MB perf.data (23 samples) ]
>   perf: util/maps.c:95: check_invariants: Assertion `map__end(prev) <= map__end(map)' failed.
>   Aborted (core dumped)
>   root@number:~#
>
> The investigation on that was happening bisected to 876e80cf83d10585
> ("perf tools: Fixup end address of modules"), and the following patches
> will plug the problems found, this patch is just legwork on that
> direction.
>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: James Clark <james.clark@linaro.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Stephane Eranian <eranian@google.com>
> Link: https://lore.kernel.org/lkml/Z74V0hZXrTLM6VIJ@x1
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  tools/perf/util/maps.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index 09c9cc326c08d435..e21d29f5df01c6f7 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
> @@ -428,11 +428,29 @@ static unsigned int maps__by_name_index(const struct maps *maps, const struct ma
>         return -1;
>  }
>
> +static void map__set_kmap(struct map *map, struct maps *maps)

I'm not sure this is the right function name. The kmap is created when
the map is created, so perhaps `map__set_kmap_maps(..)` is more
accurate.

Fwiw, kmap is:
```
#define KMAP_NAME_LEN 256

struct kmap {
struct ref_reloc_sym *ref_reloc_sym;
struct maps      *kmaps;
char      name[KMAP_NAME_LEN];
};
```

The name is pretty chunky, perhaps it should be a strdup-ed pointer.
ref_reloc_sym is used in the context of a machine, so perhaps there
should be a hashmap from map to ref_reloc_sym.
The kmaps variable seems to largely be unnecessary, things like
addr_location and map_symbol carry around a map and the associated
maps, so when kmaps is needed it was probably already one of the
arguments to the calling function and could have just been passed
through.
So I think there is a cleanup to:
1) remove kmaps - just pass down an associated maps
2) make ref_reloc_sym and name things looked up from a hashmap
3) get rid of kmap altogether so that invariants don't need to be
maintained and issues like this shouldn't repeat.

Thanks,
Ian

> +{
> +       struct dso *dso;
> +
> +       if (map == NULL)
> +               return;
> +
> +       dso = map__dso(map);
> +
> +       if (dso && dso__kernel(dso)) {
> +                struct kmap *kmap = map__kmap(map);
> +
> +                if (kmap)
> +                        kmap->kmaps = maps;
> +                else
> +                        pr_err("Internal error: kernel dso with non kernel map\n");
> +        }
> +}
> +
>  static int __maps__insert(struct maps *maps, struct map *new)
>  {
>         struct map **maps_by_address = maps__maps_by_address(maps);
>         struct map **maps_by_name = maps__maps_by_name(maps);
> -       const struct dso *dso = map__dso(new);
>         unsigned int nr_maps = maps__nr_maps(maps);
>         unsigned int nr_allocate = RC_CHK_ACCESS(maps)->nr_maps_allocated;
>
> @@ -483,14 +501,9 @@ static int __maps__insert(struct maps *maps, struct map *new)
>         }
>         if (map__end(new) < map__start(new))
>                 RC_CHK_ACCESS(maps)->ends_broken = true;
> -       if (dso && dso__kernel(dso)) {
> -               struct kmap *kmap = map__kmap(new);
>
> -               if (kmap)
> -                       kmap->kmaps = maps;
> -               else
> -                       pr_err("Internal error: kernel dso with non kernel map\n");
> -       }
> +       map__set_kmap(new, maps);
> +
>         return 0;
>  }
>
> --
> 2.47.0
>

Re: [PATCH 1/6] perf maps: Introduce map__set_kmap() for kernel maps

[Ian Rogers]

On Thu, Feb 27, 2025 at 3:54 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> On Thu, Feb 27, 2025, 8:27 PM Ian Rogers <irogers@google.com> wrote:
>>
>> On Thu, Feb 27, 2025 at 1:55 PM Arnaldo Carvalho de Melo
>> <acme@kernel.org> wrote:
>> >
>> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
>> >
>> > We need to set it in other places than __maps__insert(), so that we can
>> > have access to the 'struct kmap' from a kernel 'struct map'.
>> >
>> > When building perf with 'DEBUG=1' we can notice it failing a consistency
>> > check done in the check_invariants() function:
>> >
>> >   root@number:~# perf record -- perf test -w offcpu
>> >   [ perf record: Woken up 1 times to write data ]
>> >   [ perf record: Captured and wrote 0.040 MB perf.data (23 samples) ]
>> >   perf: util/maps.c:95: check_invariants: Assertion `map__end(prev) <= map__end(map)' failed.
>> >   Aborted (core dumped)
>> >   root@number:~#
>> >
>> > The investigation on that was happening bisected to 876e80cf83d10585
>> > ("perf tools: Fixup end address of modules"), and the following patches
>> > will plug the problems found, this patch is just legwork on that
>> > direction.
>> >
>> > Acked-by: Namhyung Kim <namhyung@kernel.org>
>> > Cc: Adrian Hunter <adrian.hunter@intel.com>
>> > Cc: Ian Rogers <irogers@google.com>
>> > Cc: Ingo Molnar <mingo@kernel.org>
>> > Cc: James Clark <james.clark@linaro.org>
>> > Cc: Jiri Olsa <jolsa@kernel.org>
>> > Cc: Kan Liang <kan.liang@linux.intel.com>
>> > Cc: Peter Zijlstra <peterz@infradead.org>
>> > Cc: Stephane Eranian <eranian@google.com>
>> > Link: https://lore.kernel.org/lkml/Z74V0hZXrTLM6VIJ@x1
>> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>> > ---
>> >  tools/perf/util/maps.c | 29 +++++++++++++++++++++--------
>> >  1 file changed, 21 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
>> > index 09c9cc326c08d435..e21d29f5df01c6f7 100644
>> > --- a/tools/perf/util/maps.c
>> > +++ b/tools/perf/util/maps.c
>> > @@ -428,11 +428,29 @@ static unsigned int maps__by_name_index(const struct maps *maps, const struct ma
>> >         return -1;
>> >  }
>> >
>> > +static void map__set_kmap(struct map *map, struct maps *maps)
>>
>> I'm not sure this is the right function name. The kmap is created when
>> the map is created, so perhaps `map__set_kmap_maps(..)` is more
>> accurate.
>>
>> Fwiw, kmap is:
>> ```
>> #define KMAP_NAME_LEN 256
>>
>> struct kmap {
>> struct ref_reloc_sym *ref_reloc_sym;
>> struct maps      *kmaps;
>> char      name[KMAP_NAME_LEN];
>> };
>> ```
>>
>> The name is pretty chunky, perhaps it should be a strdup-ed pointer.
>> ref_reloc_sym is used in the context of a machine, so perhaps there
>> should be a hashmap from map to ref_reloc_sym.
>> The kmaps variable seems to largely be unnecessary, things like
>> addr_location and map_symbol carry around a map and the associated
>> maps, so when kmaps is needed it was probably already one of the
>> arguments to the calling function and could have just been passed
>> through.
>> So I think there is a cleanup to:
>> 1) remove kmaps - just pass down an associated maps
>> 2) make ref_reloc_sym and name things looked up from a hashmap
>> 3) get rid of kmap altogether so that invariants don't need to be
>> maintained and issues like this shouldn't repeat.
>
>
> Can we work on these naming and further cleanups on top of this series?

The intention for the fwiw on the kmap stuff was to imply it is really
doing more than what this series does and can be follow up. The
function name is up to you and Namhyung as to whether it is worth
fixing in the series.

Thanks,
Ian

[PATCH 2/6] perf maps: Set the kmaps for newly created/added kernel maps

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

When using __maps__insert_sorted() the map kmaps field needs to be
initialized, as we need kernel maps to work with map__kmap().

Fix it by using the newly introduced map__set_kmap() method.

Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Link: https://lore.kernel.org/lkml/Z74V0hZXrTLM6VIJ@x1
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/maps.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
index 51b507233269d8b4..8c5f5d79cd24a6bc 100644
--- a/tools/perf/util/maps.c
+++ b/tools/perf/util/maps.c
@@ -798,6 +798,9 @@ static int __maps__insert_sorted(struct maps *maps, unsigned int first_after_ind
 	}
 	RC_CHK_ACCESS(maps)->nr_maps = nr_maps + to_add;
 	maps__set_maps_by_name_sorted(maps, false);
+	map__set_kmap_maps(new1, maps);
+	map__set_kmap_maps(new2, maps);
+
 	check_invariants(maps);
 	return 0;
 }
-- 
2.48.1