lkml.org 
[lkml]   [2013]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH 08/14] perf report: Cache cumulative callchains
    Date
    Hi Rodrigo,

    On Thu, 31 Oct 2013 11:13:34 +0000, Rodrigo Campos wrote:
    > On Thu, Oct 31, 2013 at 03:56:10PM +0900, Namhyung Kim wrote:
    >> From: Namhyung Kim <namhyung.kim@lge.com>
    >> /*
    >> + * This is for detecting cycles or recursions so that they're
    >> + * cumulated only one time to prevent entries more than 100%
    >> + * overhead.
    >> + */
    >> + ccache = malloc(sizeof(*ccache) * PERF_MAX_STACK_DEPTH);
    >> + if (ccache == NULL)
    >> + return -ENOMEM;
    >> +
    >> + node = callchain_cursor_current(&callchain_cursor);
    >> + if (node == NULL)
    >> + return 0;
    >
    > Here you return without assigning iter->priv nor iter->priv->dso iter->priv->sym

    Right! I forgot to set iter->priv to ccache in this case.

    >
    >> +
    >> + ccache[0].dso = node->map->dso;
    >> + ccache[0].sym = node->sym;
    >> +
    >> + iter->priv = ccache;
    >> + iter->curr = 1;
    >
    > Because the assignment is done here.
    >
    >> +
    >> + /*
    >> * The first callchain node always contains same information
    >> * as a hist entry itself. So skip it in order to prevent
    >> * double accounting.
    >> @@ -501,8 +528,29 @@ iter_add_next_cumulative_entry(struct add_entry_iter *iter,
    >> {
    >> struct perf_evsel *evsel = iter->evsel;
    >> struct perf_sample *sample = iter->sample;
    >> + struct cumulative_cache *ccache = iter->priv;
    >> struct hist_entry *he;
    >> int err = 0;
    >> + int i;
    >> +
    >> + /*
    >> + * Check if there's duplicate entries in the callchain.
    >> + * It's possible that it has cycles or recursive calls.
    >> + */
    >> + for (i = 0; i < iter->curr; i++) {
    >> + if (sort__has_sym) {
    >> + if (ccache[i].sym == al->sym)
    >> + return 0;
    >> + } else {
    >> + /* Not much we can do - just compare the dso. */
    >> + if (ccache[i].dso == al->map->dso)
    >
    > sym and dso are used here
    >
    >> + return 0;
    >> + }
    >> + }
    >> +
    >> + ccache[i].dso = al->map->dso;
    >> + ccache[i].sym = al->sym;
    >> + iter->curr++;
    >>
    >> he = __hists__add_entry(&evsel->hists, al, iter->parent, NULL, NULL,
    >> sample->period, sample->weight,
    >> @@ -538,6 +586,7 @@ iter_finish_cumulative_entry(struct add_entry_iter *iter,
    >> evsel->hists.stats.total_period += sample->period;
    >> hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE);
    >>
    >> + free(iter->priv);
    >
    > And here I'm seeing a double free when trying the patchset with other examples.
    > I added a printf to the "if (node == NULL)" case and I'm hitting it. So it seems
    > to me that, when reusing the entry, every user is freeing it and then the double
    > free.
    >
    > This is my first time looking at perf code, so I might be missing LOT of things,
    > sorry in advance :)

    Don't say sorry! You're very helpful and found a real bug!

    >
    > I tried copying the dso and sym to the new allocated mem (and assigning
    > iter->priv = ccache before the return if "node == NULL"), as shown in the
    > attached patch, but when running with valgrind it also added some invalid reads
    > and segfaults (without valgrind it didn't segfault, but I must be "lucky").
    >
    > So if there is no node (node == NULL) and we cannot read the dso and sym from
    > the current values of iter->priv (they show invalid reads in valgrind), I'm not
    > sure where can we read them. And, IIUC, we should initialize them because they
    > are used later. So maybe there are only some cases where we can read iter->priv
    > and for the other cases just initialize to something (although doesn't feel
    > possible because it's the dso and sym) ? Or should we read/copy them from some
    > other place (maybe before some other thing is free'd) ? Or maybe forget about
    > the malloc when node == NULL and just use iter->priv and the free shouldn't be
    > executed till iter->curr == 1 ? I added that if for the free, but didn't help.
    > Although I didn't really check how iter->curr is used. What am I missing ?

    If node == NULL, it means there no valid callchains so no need to go in
    the loop - iter_next_cumulative_entry() returns 0 so iter_add_next_
    cumulative_entry() never called. So don't worry about the sym and dso
    in this case.

    The problem is for freeing iter->priv unconditionally. Since it has
    previous ccache pointer (which already freed) it can lead to a double
    free if the next entry has no valid callchains.

    >
    > I'm not really sure which is the fix for this. Also just in case I tried
    > assigning "iter->priv = NULL" after it's free'd and it """fixes""" it.

    I think the right fix is assigning "iter->priv = NULL" as you said. But
    I changed this patch a bit for v3 so need to check it again.
    >
    > Just reverting the patch (reverts without conflict) also solves the double free
    > problem for me (although it probably introduces the problem the patch tries to
    > fix =) and seems to make valgrind happy too.
    >
    > Thanks a lot and sorry again if I'm completely missing some "rules/invariants",
    > I'm really new to perf :)

    You didn't miss anything and I'd really appreciate your review. :)

    Thanks,
    Namhyung


    \
     
     \ /
      Last update: 2013-11-01 08:21    [W:4.073 / U:2.188 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site