Re: [PATCH perf/core 17/22] perf: Fix __machine__addnew_vdso to put dso after add to dsos

Arnaldo Carvalho de Melo
2015-12-09T14:38:51Z

Em Wed, Dec 09, 2015 at 11:11:25AM +0900, Masami Hiramatsu escreveu:
> Fix __machine__addnew_vdso to put dso after add to dsos because
> the dso is already gotten by the dsos via __dsos__add().
>
> This function is called finally from machine__findnew_vdso()
> which locks machine->dsos.lock. And before unlock it, the
> function gets the dso's refcnt. Thus we can ensure that the
> dso is not removed from the machine while this operation,
> and we don't need to get the dso except for the machine->dsos.
>
> refcnt debugger shows:
> -----
> $ ./perf top --stdio -v (note: run by non-root user)
> [...]
> ==== [3] ====
> Unreclaimed dso@0x27a0a30
> Refcount +1 => 1 at
> ./perf(dso__new+0x2bc) [0x4a778c]
> ./perf(machine__findnew_vdso+0x272) [0x4e8792]
> ./perf(map__new+0x2db) [0x4bfb4b]
> ./perf(machine__process_mmap2_event+0xf3) [0x4bda33]
> ./perf(perf_event__synthesize_mmap_events+0x364) [0x484e74]
> ./perf(perf_event__synthesize_threads+0x3ee) [0x48583e]
> ./perf(cmd_top+0xdc2) [0x43cfb2]
> ./perf() [0x47ba35]
> ./perf(main+0x617) [0x4225b7]
> /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2b01387af5]
> ./perf() [0x42272d]
> Refcount +1 => 2 at
> ./perf(machine__findnew_vdso+0x289) [0x4e87a9]
> ./perf(map__new+0x2db) [0x4bfb4b]
> ./perf(machine__process_mmap2_event+0xf3) [0x4bda33]
> ./perf(perf_event__synthesize_mmap_events+0x364) [0x484e74]
> ./perf(perf_event__synthesize_threads+0x3ee) [0x48583e]
> ./perf(cmd_top+0xdc2) [0x43cfb2]
> ./perf() [0x47ba35]
> ./perf(main+0x617) [0x4225b7]
> /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2b01387af5]
> ./perf() [0x42272d]
> Refcount +1 => 3 at
> ./perf(dso__get+0x32) [0x4a7b52]
> ./perf(machine__findnew_vdso+0xc1) [0x4e85e1]
> ./perf(map__new+0x2db) [0x4bfb4b]
> ./perf(machine__process_mmap2_event+0xf3) [0x4bda33]
> ./perf(perf_event__synthesize_mmap_events+0x364) [0x484e74]
> ./perf(perf_event__synthesize_threads+0x3ee) [0x48583e]
> ./perf(cmd_top+0xdc2) [0x43cfb2]
> ./perf() [0x47ba35]
> ./perf(main+0x617) [0x4225b7]
> /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f2b01387af5]
> ./perf() [0x42272d]
> [...]
> -----
>
> The log shows that the machine__findnew_vdso gets a dso
> so many unnaturally. I've traced the code and found this
> bug.
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
> tools/perf/util/vdso.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/perf/util/vdso.c b/tools/perf/util/vdso.c
> index 44d440d..fea0d18 100644
> --- a/tools/perf/util/vdso.c
> +++ b/tools/perf/util/vdso.c
> @@ -130,6 +130,8 @@ static struct dso *__machine__addnew_vdso(struct machine *machine, const char *s
> __dsos__add(&machine->dsos, dso);
> dso__set_long_name(dso, long_name, false);
> }
> + /* Put the dso here because it is already gotten by __dsos__add */
> + dso__put(dso);
>
> return dso;
> }

We cannot put it here, because we're returning a pointer to it, so,
whoever receives this pointer, receives a recfount with it, that it, in
turn, should put.

And indeed, this dso adding code is confusing, will have to look at it
harder :-\

- Arnaldo