linux-kernel - Re: [PATCH 1/6] perf maps: Introduce map__set

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fUd_0Pf1ceo7LFws_HBNdOxkSJYMDrGrKLd1_CChu5NxQ@mail.gmail.com>
Date: Thu, 27 Feb 2025 17:16:41 -0800
From: Ian Rogers <irogers@...gle.com>
To: Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, Ingo Molnar <mingo@...nel.org>, 
	Thomas Gleixner <tglx@...utronix.de>, James Clark <james.clark@...aro.org>, 
	Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>, 
	Kan Liang <kan.liang@...ux.intel.com>, Clark Williams <williams@...hat.com>, 
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, 
	linux-perf-users <linux-perf-users@...r.kernel.org>, 
	Arnaldo Carvalho de Melo <acme@...hat.com>, Peter Zijlstra <peterz@...radead.org>, 
	Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 1/6] perf maps: Introduce map__set_kmap() for kernel maps

On Thu, Feb 27, 2025 at 3:54 PM Arnaldo Carvalho de Melo
<arnaldo.melo@...il.com> wrote:
>
> On Thu, Feb 27, 2025, 8:27 PM Ian Rogers <irogers@...gle.com> wrote:
>>
>> On Thu, Feb 27, 2025 at 1:55 PM Arnaldo Carvalho de Melo
>> <acme@...nel.org> wrote:
>> >
>> > From: Arnaldo Carvalho de Melo <acme@...hat.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@...ber:~# 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@...ber:~#
>> >
>> > 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@...nel.org>
>> > Cc: Adrian Hunter <adrian.hunter@...el.com>
>> > Cc: Ian Rogers <irogers@...gle.com>
>> > Cc: Ingo Molnar <mingo@...nel.org>
>> > Cc: James Clark <james.clark@...aro.org>
>> > Cc: Jiri Olsa <jolsa@...nel.org>
>> > Cc: Kan Liang <kan.liang@...ux.intel.com>
>> > Cc: Peter Zijlstra <peterz@...radead.org>
>> > Cc: Stephane Eranian <eranian@...gle.com>
>> > Link: https://lore.kernel.org/lkml/Z74V0hZXrTLM6VIJ@x1
>> > Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.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