On Mon, Jan 02, 2017 at 09:30:33PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jan 02, 2017 at 04:39:04PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Jan 02, 2017 at 02:36:57PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Mon, Jan 02, 2017 at 02:35:30PM -0300, Arnaldo Carvalho de Melo
> > > escreveu:
> > > > Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo
> > > > escreveu:
> > > {
> > > zfree(&iter->priv);
> > > iter->he = NULL;
> > > + map__zput(al->map);
> >
> > What this pairs to? I was expecting that since this is called via:
> >
> > hist_entry_iter__add()
> > {
> > <SNIP>
> > err2 = iter->ops->finish_entry(iter, al);
> > }
> >
> > Then it would have to match something done earlier in
> > hist_entry_iter__add(), most likely by some iter->ops->() method, but I
> > couldn'd find anything to that extent, can you clarify?
>
> With the following patch it has been running all day, care to explain
> why it is needed? I need to run this on valgrind or with Masami's
> refcount debugger to get more clues :-\
>
> - Arnaldo
>
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 72f5c82798e9..c27bda16e9cd 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -980,7 +980,6 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
> {
> zfree(&iter->priv);
> iter->he = NULL;
> - map__zput(al->map);
>
> return 0;
> }
Thanks for taking the time to debug it this far. I certainly appreciate
it.
The map_zput() in iter_finish_cumulative_entry() was intended to offset
the map_get() in iter_next_cumulative_entry() -> fill_callchain_info().
The call to fill_callchain_info should perform a map__get() on the
addr_location that gets passed in. However, it looks like I mistakenly
assumed that fill_callcahin_info would always get called from
iter_next_cumulative_entry when clearly that doesn't happen if
callchain_cursor_current returns a NULL node.
Looking back, it's not entirely clear to me that the map__get() in
fill_callchain_info is necessary either, since its only caller is the
hist_iter_cumulative used by hist_entry_iter__add.
Let me try a version of this patch that dispenses with the code in both
fill_callchain_info() and iter_finish_cumulative_entry. However, before
I do that I'll make sure I can figure out how to reproduce the core that
you were seeing. I don't want to send you yet another patch that
doesn't run. The busy system may be the clue I needed. I had been
running these on a mostly idle system.
Thanks again, and apologies for all of the inconvenience.
-K
