Hi Namhyung,
Thanks for looking this over.
On Fri, Oct 07, 2016 at 11:22:00AM +0900, Namhyung Kim wrote:
> On Wed, Oct 05, 2016 at 08:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sat, Oct 01, 2016 at 08:13:36PM -0700, Krister Johansen escreveu:
> > > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > > index 07fd30b..15c89b2 100644
> > > --- a/tools/perf/util/callchain.c
> > > +++ b/tools/perf/util/callchain.c
> > > @@ -439,7 +439,7 @@ fill_node(struct callchain_node *node, struct
> > > callchain_cursor *cursor)
> > > }
> > > call->ip = cursor_node->ip;
> > > call->ms.sym = cursor_node->sym;
> > > - call->ms.map = cursor_node->map;
> > > + call->ms.map = map__get(cursor_node->map);
> > > list_add_tail(&call->list, &node->val);
> > >
> > > callchain_cursor_advance(cursor);
> > > @@ -464,6 +464,7 @@ add_child(struct callchain_node *parent,
> > >
> > > list_for_each_entry_safe(call, tmp, &new->val, list) {
> > > list_del(&call->list);
> > > + map__zput(call->ms.map);
> > > free(call);
> > > }
> > > free(new);
> > > @@ -732,6 +733,7 @@ merge_chain_branch(struct callchain_cursor *cursor,
> > > callchain_cursor_append(cursor, list->ip,
> > > list->ms.map, list->ms.sym);
> > > list_del(&list->list);
> > > + map__zput(list->ms.map);
> > > free(list);
> > > }
> > >
> > > @@ -780,7 +782,8 @@ int callchain_cursor_append(struct callchain_cursor
> > > *cursor,
> > > }
> > >
> > > node->ip = ip;
> > > - node->map = map;
> > > + map__zput(node->map);
> > > + node->map = map__get(map);
> > > node->sym = sym;
> > >
> > > cursor->nr++;
> > > @@ -830,6 +833,8 @@ int fill_callchain_info(struct addr_location *al,
> > > struct callchain_cursor_node *
> > > goto out;
> > > }
> > >
> > > + map__get(al->map);
> > > +
> > > if (al->map->groups == &al->machine->kmaps) {
> > > if (machine__is_host(al->machine)) {
> > > al->cpumode = PERF_RECORD_MISC_KERNEL;
> > > @@ -947,11 +952,13 @@ static void free_callchain_node(struct
> > > callchain_node *node)
> > >
> > > list_for_each_entry_safe(list, tmp, &node->parent_val, list) {
> > > list_del(&list->list);
> > > + map__zput(list->ms.map);
> > > free(list);
> > > }
> > >
> > > list_for_each_entry_safe(list, tmp, &node->val, list) {
> > > list_del(&list->list);
> > > + map__zput(list->ms.map);
> > > free(list);
> > > }
> > >
> > > @@ -1035,6 +1042,7 @@ int callchain_node__make_parent_list(struct
> > > callchain_node *node)
> > > out:
> > > list_for_each_entry_safe(chain, new, &head, list) {
> > > list_del(&chain->list);
> > > + map__zput(chain->ms.map);
>
> I think you need to grab the refcnt in the "while (parent)" loop above.
Thanks; good catch. I'll fix this.
> > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > index b02992e..f8335e8 100644
> > > --- a/tools/perf/util/hist.c
> > > +++ b/tools/perf/util/hist.c
> > > @@ -1,6 +1,7 @@
> > > #include "util.h"
> > > #include "build-id.h"
> > > #include "hist.h"
> > > +#include "map.h"
> > > #include "session.h"
> > > #include "sort.h"
> > > #include "evlist.h"
> > > @@ -970,6 +971,8 @@ iter_add_next_cumulative_entry(struct hist_entry_iter
> > > *iter,
> > >
> > > if (symbol_conf.use_callchain)
> > > callchain_append(he->callchain, &cursor, sample->period);
> > > + /* Cleanup temporary cursor. */
> > > + callchain_cursor_snapshot_rele(&cursor);
>
> This callchain shotshot is used in a short period of time, and it's
> guaranteed that the maps in callchains will not freed due to refcnt in
> the orignal callchain cursor. So I think we can skip to get/put
> refcnt on the snapshot cursor. Also "rele" seems not a good name..
Ok. I'll remove the get/put from the snapshot stuff, and will excise the
rele function.
> > > return 0;
> > > }
> > >
> > > @@ -979,6 +982,7 @@ iter_finish_cumulative_entry(struct hist_entry_iter
> > > *iter,
> > > {
> > > zfree(&iter->priv);
> > > iter->he = NULL;
> > > + map__zput(al->map);
>
> What is this needed? Why other places like iter_finish_normal_entry
> isn't?
I put a map__zput() here because iter_next_cumulative_entry() calls
fill_callchain_info(), which takes a reference on the al->map in the
struct addr_location that it returns. I had forgotten all of this by
the time you asked. When I went back to work out my rationale, I
stumbled across addr_location__put(). Think it would make sense to
relocate the put of al->map there instead?
Thanks for the feedback. I'll send out a v2 once I get your changes
incorporated and tested.
-K
