Re: [PATCH v3 05/22] perf maps: Use a pointer for kmaps

From: Arnaldo Carvalho de Melo
Date: Fri Feb 11 2022 - 12:23:12 EST

Next message: Borislav Petkov: "Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures"
Previous message: Sultan Alsawaf: "Re: [PATCH v7] random: defer fast pool mixing to worker"
In reply to: Ian Rogers: "[PATCH v3 05/22] perf maps: Use a pointer for kmaps"
Next in thread: Arnaldo Carvalho de Melo: "Re: [PATCH v3 05/22] perf maps: Use a pointer for kmaps"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Em Fri, Feb 11, 2022 at 02:33:58AM -0800, Ian Rogers escreveu:
> struct maps is reference counted, using a pointer is more idiomatic.

So, I tried to apply this after adding this to the cset comming log to
make sure reviewers know that this is just a clarifying comming, no code
change:

Committer notes:

Definition of machine__kernel_maps(machine), the replacement of &machine->kmaps

static inline
struct maps *machine__kernel_maps(struct machine *machine)
{
return machine->kmaps;
}

but then when building on a f34 system I got:

CC /tmp/build/perf/bench/inject-buildid.o
In file included from /var/home/acme/git/perf/tools/perf/util/build-id.h:10,
from /var/home/acme/git/perf/tools/perf/util/dso.h:13,
from tests/vmlinux-kallsyms.c:8:
In function ‘machine__kernel_maps’,
inlined from ‘test__vmlinux_matches_kallsyms’ at tests/vmlinux-kallsyms.c:122:22:
/var/home/acme/git/perf/tools/perf/util/machine.h:86:23: error: ‘vmlinux.kmaps’ is used uninitialized [-Werror=uninitialized]
86 | return machine->kmaps;
| ~~~~~~~^~~~~~~
tests/vmlinux-kallsyms.c: In function ‘test__vmlinux_matches_kallsyms’:
tests/vmlinux-kallsyms.c:121:34: note: ‘vmlinux’ declared here
121 | struct machine kallsyms, vmlinux;
| ^~~~~~~
cc1: all warnings being treated as errors
make[4]: *** [/var/home/acme/git/perf/tools/build/Makefile.build:96: /tmp/build/perf/tests/vmlinux-kallsyms.o] Error 1
make[4]: *** Waiting for unfinished jobs....
CC /tmp/build/perf/util/config.o
CC /tmp/build/perf/arch/x86/util/archinsn.o
CC /tmp/build/perf/arch/x86/util/intel-pt.o
CC /tmp/build/perf/arch/x86/util/intel-bts.o
CC /tmp/build/perf/util/db-export.o
CC /tmp/build/perf/util/event.o
make[3]: *** [/var/home/acme/git/perf/tools/build/Makefile.build:139: tests] Error 2
make[3]: *** Waiting for unfinished jobs....

Can you please take a look at that?

- Arnaldo

> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
> tools/perf/arch/x86/util/event.c | 2 +-
> tools/perf/tests/vmlinux-kallsyms.c | 4 +--
> tools/perf/util/bpf-event.c | 2 +-
> tools/perf/util/callchain.c | 2 +-
> tools/perf/util/event.c | 6 ++---
> tools/perf/util/machine.c | 38 ++++++++++++++++-------------
> tools/perf/util/machine.h | 8 +++---
> tools/perf/util/probe-event.c | 2 +-
> 8 files changed, 34 insertions(+), 30 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/event.c b/tools/perf/arch/x86/util/event.c
> index 9b31734ee968..e670f3547581 100644
> --- a/tools/perf/arch/x86/util/event.c
> +++ b/tools/perf/arch/x86/util/event.c
> @@ -18,7 +18,7 @@ int perf_event__synthesize_extra_kmaps(struct perf_tool *tool,
> {
> int rc = 0;
> struct map *pos;
> - struct maps *kmaps = &machine->kmaps;
> + struct maps *kmaps = machine__kernel_maps(machine);
> union perf_event *event = zalloc(sizeof(event->mmap) +
> machine->id_hdr_size);
>
> diff --git a/tools/perf/tests/vmlinux-kallsyms.c b/tools/perf/tests/vmlinux-kallsyms.c
> index e80df13c0420..84bf5f640065 100644
> --- a/tools/perf/tests/vmlinux-kallsyms.c
> +++ b/tools/perf/tests/vmlinux-kallsyms.c
> @@ -293,7 +293,7 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
> * so use the short name, less descriptive but the same ("[kernel]" in
> * both cases.
> */
> - pair = maps__find_by_name(&kallsyms.kmaps, (map->dso->kernel ?
> + pair = maps__find_by_name(kallsyms.kmaps, (map->dso->kernel ?
> map->dso->short_name :
> map->dso->name));
> if (pair) {
> @@ -315,7 +315,7 @@ static int test__vmlinux_matches_kallsyms(struct test_suite *test __maybe_unused
> mem_start = vmlinux_map->unmap_ip(vmlinux_map, map->start);
> mem_end = vmlinux_map->unmap_ip(vmlinux_map, map->end);
>
> - pair = maps__find(&kallsyms.kmaps, mem_start);
> + pair = maps__find(kallsyms.kmaps, mem_start);
> if (pair == NULL || pair->priv)
> continue;
>
> diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
> index a517eaa51eb3..33257b594a71 100644
> --- a/tools/perf/util/bpf-event.c
> +++ b/tools/perf/util/bpf-event.c
> @@ -92,7 +92,7 @@ static int machine__process_bpf_event_load(struct machine *machine,
> for (i = 0; i < info_linear->info.nr_jited_ksyms; i++) {
> u64 *addrs = (u64 *)(uintptr_t)(info_linear->info.jited_ksyms);
> u64 addr = addrs[i];
> - struct map *map = maps__find(&machine->kmaps, addr);
> + struct map *map = maps__find(machine__kernel_maps(machine), addr);
>
> if (map) {
> map->dso->binary_type = DSO_BINARY_TYPE__BPF_PROG_INFO;
> diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> index 131207b91d15..5c27a4b2e7a7 100644
> --- a/tools/perf/util/callchain.c
> +++ b/tools/perf/util/callchain.c
> @@ -1119,7 +1119,7 @@ int fill_callchain_info(struct addr_location *al, struct callchain_cursor_node *
> goto out;
> }
>
> - if (al->maps == &al->maps->machine->kmaps) {
> + if (al->maps == machine__kernel_maps(al->maps->machine)) {
> if (machine__is_host(al->maps->machine)) {
> al->cpumode = PERF_RECORD_MISC_KERNEL;
> al->level = 'k';
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index fe24801f8e9f..6439c888ae38 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -484,7 +484,7 @@ size_t perf_event__fprintf_text_poke(union perf_event *event, struct machine *ma
> if (machine) {
> struct addr_location al;
>
> - al.map = maps__find(&machine->kmaps, tp->addr);
> + al.map = maps__find(machine__kernel_maps(machine), tp->addr);
> if (al.map && map__load(al.map) >= 0) {
> al.addr = al.map->map_ip(al.map, tp->addr);
> al.sym = map__find_symbol(al.map, al.addr);
> @@ -587,13 +587,13 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
>
> if (cpumode == PERF_RECORD_MISC_KERNEL && perf_host) {
> al->level = 'k';
> - al->maps = maps = &machine->kmaps;
> + al->maps = maps = machine__kernel_maps(machine);
> load_map = true;
> } else if (cpumode == PERF_RECORD_MISC_USER && perf_host) {
> al->level = '.';
> } else if (cpumode == PERF_RECORD_MISC_GUEST_KERNEL && perf_guest) {
> al->level = 'g';
> - al->maps = maps = &machine->kmaps;
> + al->maps = maps = machine__kernel_maps(machine);
> load_map = true;
> } else if (cpumode == PERF_RECORD_MISC_GUEST_USER && perf_guest) {
> al->level = 'u';
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index f70ba56912d4..57fbdba66425 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -89,7 +89,10 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
> int err = -ENOMEM;
>
> memset(machine, 0, sizeof(*machine));
> - maps__init(&machine->kmaps, machine);
> + machine->kmaps = maps__new(machine);
> + if (machine->kmaps == NULL)
> + return -ENOMEM;
> +
> RB_CLEAR_NODE(&machine->rb_node);
> dsos__init(&machine->dsos);
>
> @@ -108,7 +111,7 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
>
> machine->root_dir = strdup(root_dir);
> if (machine->root_dir == NULL)
> - return -ENOMEM;
> + goto out;
>
> if (machine__set_mmap_name(machine))
> goto out;
> @@ -131,6 +134,7 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
>
> out:
> if (err) {
> + zfree(&machine->kmaps);
> zfree(&machine->root_dir);
> zfree(&machine->mmap_name);
> }
> @@ -220,7 +224,7 @@ void machine__exit(struct machine *machine)
> return;
>
> machine__destroy_kernel_maps(machine);
> - maps__exit(&machine->kmaps);
> + maps__delete(machine->kmaps);
> dsos__exit(&machine->dsos);
> machine__exit_vdso(machine);
> zfree(&machine->root_dir);
> @@ -778,7 +782,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
> struct perf_sample *sample __maybe_unused)
> {
> struct symbol *sym;
> - struct map *map = maps__find(&machine->kmaps, event->ksymbol.addr);
> + struct map *map = maps__find(machine__kernel_maps(machine), event->ksymbol.addr);
>
> if (!map) {
> struct dso *dso = dso__new(event->ksymbol.name);
> @@ -801,7 +805,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
>
> map->start = event->ksymbol.addr;
> map->end = map->start + event->ksymbol.len;
> - maps__insert(&machine->kmaps, map);
> + maps__insert(machine__kernel_maps(machine), map);
> map__put(map);
> dso__set_loaded(dso);
>
> @@ -827,12 +831,12 @@ static int machine__process_ksymbol_unregister(struct machine *machine,
> struct symbol *sym;
> struct map *map;
>
> - map = maps__find(&machine->kmaps, event->ksymbol.addr);
> + map = maps__find(machine__kernel_maps(machine), event->ksymbol.addr);
> if (!map)
> return 0;
>
> if (map != machine->vmlinux_map)
> - maps__remove(&machine->kmaps, map);
> + maps__remove(machine__kernel_maps(machine), map);
> else {
> sym = dso__find_symbol(map->dso, map->map_ip(map, map->start));
> if (sym)
> @@ -858,7 +862,7 @@ int machine__process_ksymbol(struct machine *machine __maybe_unused,
> int machine__process_text_poke(struct machine *machine, union perf_event *event,
> struct perf_sample *sample __maybe_unused)
> {
> - struct map *map = maps__find(&machine->kmaps, event->text_poke.addr);
> + struct map *map = maps__find(machine__kernel_maps(machine), event->text_poke.addr);
> u8 cpumode = event->header.misc & PERF_RECORD_MISC_CPUMODE_MASK;
>
> if (dump_trace)
> @@ -914,7 +918,7 @@ static struct map *machine__addnew_module_map(struct machine *machine, u64 start
> if (map == NULL)
> goto out;
>
> - maps__insert(&machine->kmaps, map);
> + maps__insert(machine__kernel_maps(machine), map);
>
> /* Put the map here because maps__insert already got it */
> map__put(map);
> @@ -1100,7 +1104,7 @@ int machine__create_extra_kernel_map(struct machine *machine,
>
> strlcpy(kmap->name, xm->name, KMAP_NAME_LEN);
>
> - maps__insert(&machine->kmaps, map);
> + maps__insert(machine__kernel_maps(machine), map);
>
> pr_debug2("Added extra kernel map %s %" PRIx64 "-%" PRIx64 "\n",
> kmap->name, map->start, map->end);
> @@ -1145,7 +1149,7 @@ static u64 find_entry_trampoline(struct dso *dso)
> int machine__map_x86_64_entry_trampolines(struct machine *machine,
> struct dso *kernel)
> {
> - struct maps *kmaps = &machine->kmaps;
> + struct maps *kmaps = machine__kernel_maps(machine);
> int nr_cpus_avail, cpu;
> bool found = false;
> struct map *map;
> @@ -1215,7 +1219,7 @@ __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
> return -1;
>
> machine->vmlinux_map->map_ip = machine->vmlinux_map->unmap_ip = identity__map_ip;
> - maps__insert(&machine->kmaps, machine->vmlinux_map);
> + maps__insert(machine__kernel_maps(machine), machine->vmlinux_map);
> return 0;
> }
>
> @@ -1228,7 +1232,7 @@ void machine__destroy_kernel_maps(struct machine *machine)
> return;
>
> kmap = map__kmap(map);
> - maps__remove(&machine->kmaps, map);
> + maps__remove(machine__kernel_maps(machine), map);
> if (kmap && kmap->ref_reloc_sym) {
> zfree((char **)&kmap->ref_reloc_sym->name);
> zfree(&kmap->ref_reloc_sym);
> @@ -1323,7 +1327,7 @@ int machine__load_kallsyms(struct machine *machine, const char *filename)
> * kernel, with modules between them, fixup the end of all
> * sections.
> */
> - maps__fixup_end(&machine->kmaps);
> + maps__fixup_end(machine__kernel_maps(machine));
> }
>
> return ret;
> @@ -1471,7 +1475,7 @@ static int machine__set_modules_path(struct machine *machine)
> machine->root_dir, version);
> free(version);
>
> - return maps__set_modules_path_dir(&machine->kmaps, modules_path, 0);
> + return maps__set_modules_path_dir(machine__kernel_maps(machine), modules_path, 0);
> }
> int __weak arch__fix_module_text_start(u64 *start __maybe_unused,
> u64 *size __maybe_unused,
> @@ -1544,11 +1548,11 @@ static void machine__update_kernel_mmap(struct machine *machine,
> struct map *map = machine__kernel_map(machine);
>
> map__get(map);
> - maps__remove(&machine->kmaps, map);
> + maps__remove(machine__kernel_maps(machine), map);
>
> machine__set_kernel_mmap(machine, start, end);
>
> - maps__insert(&machine->kmaps, map);
> + maps__insert(machine__kernel_maps(machine), map);
> map__put(map);
> }
>
> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> index c5a45dc8df4c..0023165422aa 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -51,7 +51,7 @@ struct machine {
> struct vdso_info *vdso_info;
> struct perf_env *env;
> struct dsos dsos;
> - struct maps kmaps;
> + struct maps *kmaps;
> struct map *vmlinux_map;
> u64 kernel_start;
> pid_t *current_tid;
> @@ -83,7 +83,7 @@ struct map *machine__kernel_map(struct machine *machine)
> static inline
> struct maps *machine__kernel_maps(struct machine *machine)
> {
> - return &machine->kmaps;
> + return machine->kmaps;
> }
>
> int machine__get_kernel_start(struct machine *machine);
> @@ -223,7 +223,7 @@ static inline
> struct symbol *machine__find_kernel_symbol(struct machine *machine, u64 addr,
> struct map **mapp)
> {
> - return maps__find_symbol(&machine->kmaps, addr, mapp);
> + return maps__find_symbol(machine->kmaps, addr, mapp);
> }
>
> static inline
> @@ -231,7 +231,7 @@ struct symbol *machine__find_kernel_symbol_by_name(struct machine *machine,
> const char *name,
> struct map **mapp)
> {
> - return maps__find_symbol_by_name(&machine->kmaps, name, mapp);
> + return maps__find_symbol_by_name(machine->kmaps, name, mapp);
> }
>
> int arch__fix_module_text_start(u64 *start, u64 *size, const char *name);
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 7444e689ece7..bc5ab782ace5 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -334,7 +334,7 @@ static int kernel_get_module_dso(const char *module, struct dso **pdso)
> char module_name[128];
>
> snprintf(module_name, sizeof(module_name), "[%s]", module);
> - map = maps__find_by_name(&host_machine->kmaps, module_name);
> + map = maps__find_by_name(machine__kernel_maps(host_machine), module_name);
> if (map) {
> dso = map->dso;
> goto found;
> --
> 2.35.1.265.g69c8d7142f-goog

--

- Arnaldo

Next message: Borislav Petkov: "Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures"
Previous message: Sultan Alsawaf: "Re: [PATCH v7] random: defer fast pool mixing to worker"
In reply to: Ian Rogers: "[PATCH v3 05/22] perf maps: Use a pointer for kmaps"
Next in thread: Arnaldo Carvalho de Melo: "Re: [PATCH v3 05/22] perf maps: Use a pointer for kmaps"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]