Ensure gets and puts are better aligned fixing reference couting
checking problems.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/machine.c | 4 +--
tools/perf/util/map.c | 1 +
tools/perf/util/symbol-elf.c | 51 ++++++++++++++++++------------------
3 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 0b8fb14f5ff6..4a4541a2e887 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -683,7 +683,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
struct perf_sample *sample __maybe_unused)
{
struct symbol *sym;
- struct dso *dso;
+ struct dso *dso = NULL;
struct map *map = maps__find(machine__kernel_maps(machine), event->ksymbol.addr);
int err = 0;
@@ -696,7 +696,6 @@ static int machine__process_ksymbol_register(struct machine *machine,
}
dso__set_kernel(dso, DSO_SPACE__KERNEL);
map = map__new2(0, dso);
- dso__put(dso);
if (!map) {
err = -ENOMEM;
goto out;
@@ -735,6 +734,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
dso__insert_symbol(dso, sym);
out:
map__put(map);
+ dso__put(dso);
return err;
}
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 117c4bb78b35..e1d14936a60d 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -200,6 +200,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
dso__set_build_id(dso, dso__bid(header_bid_dso));
dso__set_header_build_id(dso, 1);
}
+ dso__put(header_bid_dso);
}
dso__put(dso);
}
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 3be5e8d1e278..e398abfd13a0 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -1419,7 +1419,7 @@ void __weak arch__sym_update(struct symbol *s __maybe_unused,
static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
GElf_Sym *sym, GElf_Shdr *shdr,
struct maps *kmaps, struct kmap *kmap,
- struct dso **curr_dsop, struct map **curr_mapp,
+ struct dso **curr_dsop,
const char *section_name,
bool adjust_kernel_syms, bool kmodule, bool *remap_kernel,
u64 max_text_sh_offset)
@@ -1470,8 +1470,8 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
map__set_pgoff(map, shdr->sh_offset);
}
- *curr_mapp = map;
- *curr_dsop = dso;
+ dso__put(*curr_dsop);
+ *curr_dsop = dso__get(dso);
return 0;
}
@@ -1484,8 +1484,8 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
*/
if (kmodule && adjust_kernel_syms && is_exe_text(shdr->sh_flags) &&
shdr->sh_offset <= max_text_sh_offset) {
- *curr_mapp = map;
- *curr_dsop = dso;
+ dso__put(*curr_dsop);
+ *curr_dsop = dso__get(dso);
return 0;
}
@@ -1507,10 +1507,10 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
dso__set_binary_type(curr_dso, dso__binary_type(dso));
dso__set_adjust_symbols(curr_dso, dso__adjust_symbols(dso));
curr_map = map__new2(start, curr_dso);
- dso__put(curr_dso);
- if (curr_map == NULL)
+ if (curr_map == NULL) {
+ dso__put(curr_dso);
return -1;
-
+ }
if (dso__kernel(curr_dso))
map__kmap(curr_map)->kmaps = kmaps;
@@ -1524,21 +1524,15 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
dso__set_symtab_type(curr_dso, dso__symtab_type(dso));
if (maps__insert(kmaps, curr_map))
return -1;
- /*
- * Add it before we drop the reference to curr_map, i.e. while
- * we still are sure to have a reference to this DSO via
- * *curr_map->dso.
- */
dsos__add(&maps__machine(kmaps)->dsos, curr_dso);
- /* kmaps already got it */
- map__put(curr_map);
dso__set_loaded(curr_dso);
- *curr_mapp = curr_map;
+ dso__put(*curr_dsop);
*curr_dsop = curr_dso;
} else {
- *curr_dsop = map__dso(curr_map);
- map__put(curr_map);
+ dso__put(*curr_dsop);
+ *curr_dsop = dso__get(map__dso(curr_map));
}
+ map__put(curr_map);
return 0;
}
@@ -1549,11 +1543,9 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
{
struct kmap *kmap = dso__kernel(dso) ? map__kmap(map) : NULL;
struct maps *kmaps = kmap ? map__kmaps(map) : NULL;
- struct map *curr_map = map;
- struct dso *curr_dso = dso;
+ struct dso *curr_dso = NULL;
Elf_Data *symstrs, *secstrs, *secstrs_run, *secstrs_sym;
uint32_t nr_syms;
- int err = -1;
uint32_t idx;
GElf_Ehdr ehdr;
GElf_Shdr shdr;
@@ -1656,6 +1648,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
if (kmodule && adjust_kernel_syms)
max_text_sh_offset = max_text_section(runtime_ss->elf, &runtime_ss->ehdr);
+ curr_dso = dso__get(dso);
elf_symtab__for_each_symbol(syms, nr_syms, idx, sym) {
struct symbol *f;
const char *elf_name = elf_sym__name(&sym, symstrs);
@@ -1744,9 +1737,13 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
--sym.st_value;
if (dso__kernel(dso)) {
- if (dso__process_kernel_symbol(dso, map, &sym, &shdr, kmaps, kmap, &curr_dso, &curr_map,
- section_name, adjust_kernel_syms, kmodule,
- &remap_kernel, max_text_sh_offset))
+ if (dso__process_kernel_symbol(dso, map, &sym, &shdr,
+ kmaps, kmap, &curr_dso,
+ section_name,
+ adjust_kernel_syms,
+ kmodule,
+ &remap_kernel,
+ max_text_sh_offset))
goto out_elf_end;
} else if ((used_opd && runtime_ss->adjust_symbols) ||
(!used_opd && syms_ss->adjust_symbols)) {
@@ -1795,6 +1792,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
__symbols__insert(dso__symbols(curr_dso), f, dso__kernel(dso));
nr++;
}
+ dso__put(curr_dso);
/*
* For misannotated, zeroed, ASM function sizes.
@@ -1810,9 +1808,10 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
maps__fixup_end(kmaps);
}
}
- err = nr;
+ return nr;
out_elf_end:
- return err;
+ dso__put(curr_dso);
+ return -1;
}
int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
--
2.45.0.rc1.225.g2a3ae87e7f-googOn Sat, May 04, 2024 at 02:38:02PM -0700, Ian Rogers wrote:
> Ensure gets and puts are better aligned fixing reference couting
> checking problems.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/machine.c | 4 +--
> tools/perf/util/map.c | 1 +
> tools/perf/util/symbol-elf.c | 51 ++++++++++++++++++------------------
> 3 files changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 0b8fb14f5ff6..4a4541a2e887 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -683,7 +683,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
> struct perf_sample *sample __maybe_unused)
> {
> struct symbol *sym;
> - struct dso *dso;
> + struct dso *dso = NULL;
> struct map *map = maps__find(machine__kernel_maps(machine), event->ksymbol.addr);
> int err = 0;
>
> @@ -696,7 +696,6 @@ static int machine__process_ksymbol_register(struct machine *machine,
> }
> dso__set_kernel(dso, DSO_SPACE__KERNEL);
> map = map__new2(0, dso);
> - dso__put(dso);
> if (!map) {
> err = -ENOMEM;
> goto out;
> @@ -735,6 +734,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
> dso__insert_symbol(dso, sym);
> out:
> map__put(map);
> + dso__put(dso);
> return err;
> }
This seems to match the patch description, good, just aligning the puts
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 117c4bb78b35..e1d14936a60d 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -200,6 +200,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
> dso__set_build_id(dso, dso__bid(header_bid_dso));
> dso__set_header_build_id(dso, 1);
> }
> + dso__put(header_bid_dso);
> }
> dso__put(dso);
> }
But this is a missing one, so not aligning, but fixing a separate issue,
i.e. a missing put? Should go on a different patch, probably with a
Fixes.
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 3be5e8d1e278..e398abfd13a0 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -1419,7 +1419,7 @@ void __weak arch__sym_update(struct symbol *s __maybe_unused,
> static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
> GElf_Sym *sym, GElf_Shdr *shdr,
> struct maps *kmaps, struct kmap *kmap,
> - struct dso **curr_dsop, struct map **curr_mapp,
> + struct dso **curr_dsop,
This one removes an argument, so look like a separate patch as well,
needs a description of why removing the argument is the right thing to
do here.
> const char *section_name,
> bool adjust_kernel_syms, bool kmodule, bool *remap_kernel,
> u64 max_text_sh_offset)
> @@ -1470,8 +1470,8 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
> map__set_pgoff(map, shdr->sh_offset);
> }
>
> - *curr_mapp = map;
> - *curr_dsop = dso;
> + dso__put(*curr_dsop);
> + *curr_dsop = dso__get(dso);
> return 0;
> }
>
> @@ -1484,8 +1484,8 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
> */
> if (kmodule && adjust_kernel_syms && is_exe_text(shdr->sh_flags) &&
> shdr->sh_offset <= max_text_sh_offset) {
> - *curr_mapp = map;
> - *curr_dsop = dso;
> + dso__put(*curr_dsop);
> + *curr_dsop = dso__get(dso);
> return 0;
> }
>
> @@ -1507,10 +1507,10 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
> dso__set_binary_type(curr_dso, dso__binary_type(dso));
> dso__set_adjust_symbols(curr_dso, dso__adjust_symbols(dso));
> curr_map = map__new2(start, curr_dso);
> - dso__put(curr_dso);
> - if (curr_map == NULL)
> + if (curr_map == NULL) {
> + dso__put(curr_dso);
> return -1;
> -
> + }
> if (dso__kernel(curr_dso))
> map__kmap(curr_map)->kmaps = kmaps;
>
> @@ -1524,21 +1524,15 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
> dso__set_symtab_type(curr_dso, dso__symtab_type(dso));
> if (maps__insert(kmaps, curr_map))
> return -1;
> - /*
> - * Add it before we drop the reference to curr_map, i.e. while
> - * we still are sure to have a reference to this DSO via
> - * *curr_map->dso.
> - */
> dsos__add(&maps__machine(kmaps)->dsos, curr_dso);
> - /* kmaps already got it */
> - map__put(curr_map);
> dso__set_loaded(curr_dso);
> - *curr_mapp = curr_map;
> + dso__put(*curr_dsop);
> *curr_dsop = curr_dso;
> } else {
> - *curr_dsop = map__dso(curr_map);
> - map__put(curr_map);
> + dso__put(*curr_dsop);
> + *curr_dsop = dso__get(map__dso(curr_map));
> }
> + map__put(curr_map);
>
> return 0;
> }
> @@ -1549,11 +1543,9 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> {
> struct kmap *kmap = dso__kernel(dso) ? map__kmap(map) : NULL;
> struct maps *kmaps = kmap ? map__kmaps(map) : NULL;
> - struct map *curr_map = map;
> - struct dso *curr_dso = dso;
> + struct dso *curr_dso = NULL;
> Elf_Data *symstrs, *secstrs, *secstrs_run, *secstrs_sym;
> uint32_t nr_syms;
> - int err = -1;
> uint32_t idx;
> GElf_Ehdr ehdr;
> GElf_Shdr shdr;
> @@ -1656,6 +1648,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> if (kmodule && adjust_kernel_syms)
> max_text_sh_offset = max_text_section(runtime_ss->elf, &runtime_ss->ehdr);
>
> + curr_dso = dso__get(dso);
> elf_symtab__for_each_symbol(syms, nr_syms, idx, sym) {
> struct symbol *f;
> const char *elf_name = elf_sym__name(&sym, symstrs);
> @@ -1744,9 +1737,13 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> --sym.st_value;
>
> if (dso__kernel(dso)) {
> - if (dso__process_kernel_symbol(dso, map, &sym, &shdr, kmaps, kmap, &curr_dso, &curr_map,
> - section_name, adjust_kernel_syms, kmodule,
> - &remap_kernel, max_text_sh_offset))
> + if (dso__process_kernel_symbol(dso, map, &sym, &shdr,
> + kmaps, kmap, &curr_dso,
> + section_name,
> + adjust_kernel_syms,
> + kmodule,
> + &remap_kernel,
> + max_text_sh_offset))
Can you please avoid reflowing to reduce the cost of reviewing? If this
was just:
- if (dso__process_kernel_symbol(dso, map, &sym, &shdr, kmaps, kmap, &curr_dso, &curr_map,
section_name, adjust_kernel_syms, kmodule,
&remap_kernel, max_text_sh_offset))
+ if (dso__process_kernel_symbol(dso, map, &sym, &shdr, kmaps, kmap, &curr_dso,
section_name, adjust_kernel_syms, kmodule,
&remap_kernel, max_text_sh_offset))
The actual change would be quickly spotted as just the line where it
takes place gets changed.
I applied all the patches in this series but this one, please split it
as described.
The tests I run after each patch before this one passed, thanks for
addressing the issues I reported,
I'm pushing what I have to tmp.perf-tools-next,
- Arnaldo
> goto out_elf_end;
> } else if ((used_opd && runtime_ss->adjust_symbols) ||
> (!used_opd && syms_ss->adjust_symbols)) {
> @@ -1795,6 +1792,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> __symbols__insert(dso__symbols(curr_dso), f, dso__kernel(dso));
> nr++;
> }
> + dso__put(curr_dso);
>
> /*
> * For misannotated, zeroed, ASM function sizes.
> @@ -1810,9 +1808,10 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> maps__fixup_end(kmaps);
> }
> }
> - err = nr;
> + return nr;
> out_elf_end:
> - return err;
> + dso__put(curr_dso);
> + return -1;
> }
>
> int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> --
> 2.45.0.rc1.225.g2a3ae87e7f-goog
>
On Mon, May 6, 2024 at 7:44 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> On Sat, May 04, 2024 at 02:38:02PM -0700, Ian Rogers wrote:
> > Ensure gets and puts are better aligned fixing reference couting
> > checking problems.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/perf/util/machine.c | 4 +--
> > tools/perf/util/map.c | 1 +
> > tools/perf/util/symbol-elf.c | 51 ++++++++++++++++++------------------
> > 3 files changed, 28 insertions(+), 28 deletions(-)
> >
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index 0b8fb14f5ff6..4a4541a2e887 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -683,7 +683,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
> > struct perf_sample *sample __maybe_unused)
> > {
> > struct symbol *sym;
> > - struct dso *dso;
> > + struct dso *dso = NULL;
> > struct map *map = maps__find(machine__kernel_maps(machine), event->ksymbol.addr);
> > int err = 0;
> >
> > @@ -696,7 +696,6 @@ static int machine__process_ksymbol_register(struct machine *machine,
> > }
> > dso__set_kernel(dso, DSO_SPACE__KERNEL);
> > map = map__new2(0, dso);
> > - dso__put(dso);
> > if (!map) {
> > err = -ENOMEM;
> > goto out;
> > @@ -735,6 +734,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
> > dso__insert_symbol(dso, sym);
> > out:
> > map__put(map);
> > + dso__put(dso);
> > return err;
> > }
>
> This seems to match the patch description, good, just aligning the puts
>
> > diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> > index 117c4bb78b35..e1d14936a60d 100644
> > --- a/tools/perf/util/map.c
> > +++ b/tools/perf/util/map.c
> > @@ -200,6 +200,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
> > dso__set_build_id(dso, dso__bid(header_bid_dso));
> > dso__set_header_build_id(dso, 1);
> > }
> > + dso__put(header_bid_dso);
> > }
> > dso__put(dso);
> > }
>
> But this is a missing one, so not aligning, but fixing a separate issue,
> i.e. a missing put? Should go on a different patch, probably with a
> Fixes.
The Fixes would be for a patch in the series - albeit that we've spent
over half a year landing this and a month since that change landed:
https://lore.kernel.org/all/20240410064214.2755936-3-irogers@google.com/
The issue is that previously a find wouldn't do a get, so per the old
code no put is necessary. Gets were added to finds to cover the race
between a dso being returned from dsos and the item potentially being
removed from dsos.
> > diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> > index 3be5e8d1e278..e398abfd13a0 100644
> > --- a/tools/perf/util/symbol-elf.c
> > +++ b/tools/perf/util/symbol-elf.c
> > @@ -1419,7 +1419,7 @@ void __weak arch__sym_update(struct symbol *s __maybe_unused,
> > static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
> > GElf_Sym *sym, GElf_Shdr *shdr,
> > struct maps *kmaps, struct kmap *kmap,
> > - struct dso **curr_dsop, struct map **curr_mapp,
> > + struct dso **curr_dsop,
>
> This one removes an argument, so look like a separate patch as well,
> needs a description of why removing the argument is the right thing to
> do here.
>
> > const char *section_name,
> > bool adjust_kernel_syms, bool kmodule, bool *remap_kernel,
> > u64 max_text_sh_offset)
> > @@ -1470,8 +1470,8 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
> > map__set_pgoff(map, shdr->sh_offset);
> > }
> >
> > - *curr_mapp = map;
> > - *curr_dsop = dso;
> > + dso__put(*curr_dsop);
> > + *curr_dsop = dso__get(dso);
> > return 0;
> > }
> >
> > @@ -1484,8 +1484,8 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
> > */
> > if (kmodule && adjust_kernel_syms && is_exe_text(shdr->sh_flags) &&
> > shdr->sh_offset <= max_text_sh_offset) {
> > - *curr_mapp = map;
> > - *curr_dsop = dso;
> > + dso__put(*curr_dsop);
> > + *curr_dsop = dso__get(dso);
> > return 0;
> > }
> >
> > @@ -1507,10 +1507,10 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
> > dso__set_binary_type(curr_dso, dso__binary_type(dso));
> > dso__set_adjust_symbols(curr_dso, dso__adjust_symbols(dso));
> > curr_map = map__new2(start, curr_dso);
> > - dso__put(curr_dso);
> > - if (curr_map == NULL)
> > + if (curr_map == NULL) {
> > + dso__put(curr_dso);
> > return -1;
> > -
> > + }
> > if (dso__kernel(curr_dso))
> > map__kmap(curr_map)->kmaps = kmaps;
> >
> > @@ -1524,21 +1524,15 @@ static int dso__process_kernel_symbol(struct dso *dso, struct map *map,
> > dso__set_symtab_type(curr_dso, dso__symtab_type(dso));
> > if (maps__insert(kmaps, curr_map))
> > return -1;
> > - /*
> > - * Add it before we drop the reference to curr_map, i.e. while
> > - * we still are sure to have a reference to this DSO via
> > - * *curr_map->dso.
> > - */
> > dsos__add(&maps__machine(kmaps)->dsos, curr_dso);
> > - /* kmaps already got it */
> > - map__put(curr_map);
> > dso__set_loaded(curr_dso);
> > - *curr_mapp = curr_map;
> > + dso__put(*curr_dsop);
> > *curr_dsop = curr_dso;
> > } else {
> > - *curr_dsop = map__dso(curr_map);
> > - map__put(curr_map);
> > + dso__put(*curr_dsop);
> > + *curr_dsop = dso__get(map__dso(curr_map));
> > }
> > + map__put(curr_map);
> >
> > return 0;
> > }
> > @@ -1549,11 +1543,9 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> > {
> > struct kmap *kmap = dso__kernel(dso) ? map__kmap(map) : NULL;
> > struct maps *kmaps = kmap ? map__kmaps(map) : NULL;
> > - struct map *curr_map = map;
> > - struct dso *curr_dso = dso;
> > + struct dso *curr_dso = NULL;
> > Elf_Data *symstrs, *secstrs, *secstrs_run, *secstrs_sym;
> > uint32_t nr_syms;
> > - int err = -1;
> > uint32_t idx;
> > GElf_Ehdr ehdr;
> > GElf_Shdr shdr;
> > @@ -1656,6 +1648,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> > if (kmodule && adjust_kernel_syms)
> > max_text_sh_offset = max_text_section(runtime_ss->elf, &runtime_ss->ehdr);
> >
> > + curr_dso = dso__get(dso);
> > elf_symtab__for_each_symbol(syms, nr_syms, idx, sym) {
> > struct symbol *f;
> > const char *elf_name = elf_sym__name(&sym, symstrs);
> > @@ -1744,9 +1737,13 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> > --sym.st_value;
> >
> > if (dso__kernel(dso)) {
> > - if (dso__process_kernel_symbol(dso, map, &sym, &shdr, kmaps, kmap, &curr_dso, &curr_map,
> > - section_name, adjust_kernel_syms, kmodule,
> > - &remap_kernel, max_text_sh_offset))
> > + if (dso__process_kernel_symbol(dso, map, &sym, &shdr,
> > + kmaps, kmap, &curr_dso,
> > + section_name,
> > + adjust_kernel_syms,
> > + kmodule,
> > + &remap_kernel,
> > + max_text_sh_offset))
>
> Can you please avoid reflowing to reduce the cost of reviewing? If this
> was just:
>
> - if (dso__process_kernel_symbol(dso, map, &sym, &shdr, kmaps, kmap, &curr_dso, &curr_map,
> section_name, adjust_kernel_syms, kmodule,
> &remap_kernel, max_text_sh_offset))
> + if (dso__process_kernel_symbol(dso, map, &sym, &shdr, kmaps, kmap, &curr_dso,
> section_name, adjust_kernel_syms, kmodule,
> &remap_kernel, max_text_sh_offset))
>
> The actual change would be quickly spotted as just the line where it
> takes place gets changed.
>
> I applied all the patches in this series but this one, please split it
> as described.
*sigh* 28 line change...
Ian
> The tests I run after each patch before this one passed, thanks for
> addressing the issues I reported,
>
> I'm pushing what I have to tmp.perf-tools-next,
>
> - Arnaldo
>
> > goto out_elf_end;
> > } else if ((used_opd && runtime_ss->adjust_symbols) ||
> > (!used_opd && syms_ss->adjust_symbols)) {
> > @@ -1795,6 +1792,7 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> > __symbols__insert(dso__symbols(curr_dso), f, dso__kernel(dso));
> > nr++;
> > }
> > + dso__put(curr_dso);
> >
> > /*
> > * For misannotated, zeroed, ASM function sizes.
> > @@ -1810,9 +1808,10 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> > maps__fixup_end(kmaps);
> > }
> > }
> > - err = nr;
> > + return nr;
> > out_elf_end:
> > - return err;
> > + dso__put(curr_dso);
> > + return -1;
> > }
> >
> > int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> > --
> > 2.45.0.rc1.225.g2a3ae87e7f-goog
> >
On Mon, May 06, 2024 at 08:26:02AM -0700, Ian Rogers wrote:
> On Mon, May 6, 2024 at 7:44 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > On Sat, May 04, 2024 at 02:38:02PM -0700, Ian Rogers wrote:
> > > Ensure gets and puts are better aligned fixing reference couting
> > > checking problems.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > > tools/perf/util/machine.c | 4 +--
> > > tools/perf/util/map.c | 1 +
> > > tools/perf/util/symbol-elf.c | 51 ++++++++++++++++++------------------
> > > 3 files changed, 28 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > > index 0b8fb14f5ff6..4a4541a2e887 100644
> > > --- a/tools/perf/util/machine.c
> > > +++ b/tools/perf/util/machine.c
> > > @@ -683,7 +683,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
> > > struct perf_sample *sample __maybe_unused)
> > > {
> > > struct symbol *sym;
> > > - struct dso *dso;
> > > + struct dso *dso = NULL;
> > > struct map *map = maps__find(machine__kernel_maps(machine), event->ksymbol.addr);
> > > int err = 0;
> > >
> > > @@ -696,7 +696,6 @@ static int machine__process_ksymbol_register(struct machine *machine,
> > > }
> > > dso__set_kernel(dso, DSO_SPACE__KERNEL);
> > > map = map__new2(0, dso);
> > > - dso__put(dso);
> > > if (!map) {
> > > err = -ENOMEM;
> > > goto out;
> > > @@ -735,6 +734,7 @@ static int machine__process_ksymbol_register(struct machine *machine,
> > > dso__insert_symbol(dso, sym);
> > > out:
> > > map__put(map);
> > > + dso__put(dso);
> > > return err;
> > > }
> >
> > This seems to match the patch description, good, just aligning the puts
> >
> > > diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> > > index 117c4bb78b35..e1d14936a60d 100644
> > > --- a/tools/perf/util/map.c
> > > +++ b/tools/perf/util/map.c
> > > @@ -200,6 +200,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
> > > dso__set_build_id(dso, dso__bid(header_bid_dso));
> > > dso__set_header_build_id(dso, 1);
> > > }
> > > + dso__put(header_bid_dso);
> > > }
> > > dso__put(dso);
> > > }
> >
> > But this is a missing one, so not aligning, but fixing a separate issue,
> > i.e. a missing put? Should go on a different patch, probably with a
> > Fixes.
>
> The Fixes would be for a patch in the series - albeit that we've spent
Well, if it fixes something that got merged before, it should have the
Fixes, please then have it in the patch fixing it:
Fixes: f649ed80f3cabbf1 ("perf dsos: Tidy reference counting and locking")
> over half a year landing this and a month since that change landed:
> https://lore.kernel.org/all/20240410064214.2755936-3-irogers@google.com/
> The issue is that previously a find wouldn't do a get, so per the old
> code no put is necessary. Gets were added to finds to cover the race
> between a dso being returned from dsos and the item potentially being
> removed from dsos.
The last patch applied:
⬢[acme@toolbox perf-tools-next]$ git log --oneline -1
ee756ef7491eafd7 (HEAD -> perf-tools-next, perf-tools-next.korg/tmp.perf-tools-next) perf dso: Add reference count checking and accessor functions
⬢[acme@toolbox perf-tools-next]$
Needed this, which I applied and pushed to tmp.perf-tools-next:
util/symbol.c: In function ‘dso__load_bfd_symbols’:
util/symbol.c:1683:9: error: too few arguments to function ‘dso__set_adjust_symbols’
1683 | dso__set_adjust_symbols(dso);
| ^~~~~~~~~~~~~~~~~~~~~~~
In file included from util/symbol.c:21:
util/dso.h:268:20: note: declared here
268 | static inline void dso__set_adjust_symbols(struct dso *dso, bool val)
| ^~~~~~~~~~~~~~~~~~~~~~~
make[6]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:106: /tmp/tmp.ZWHbQftdN6/util/symbol.o] Error 1
MKDIR /tmp/tmp.ZWHbQftdN6/tests/workloads/
make[6]: *** Waiting for unfinished jobs....
This was updated:
- symbols__fixup_end(&dso->symbols, false);
- symbols__fixup_duplicate(&dso->symbols);
- dso->adjust_symbols = 1;
+ symbols__fixup_end(dso__symbols(dso), false);
+ symbols__fixup_duplicate(dso__symbols(dso));
+ dso__set_adjust_symbols(dso);
But not build tested with BUILD_NONDISTRO and libbfd devel files installed
(binutils-devel on fedora), I noticed it using 'make -C tools/perf build-test'.
Add the missing argument:
symbols__fixup_end(dso__symbols(dso), false);
symbols__fixup_duplicate(dso__symbols(dso));
- dso__set_adjust_symbols(dso);
+ dso__set_adjust_symbols(dso, true);
© 2016 - 2026 Red Hat, Inc.