Re: [PATCH 4/7] perf hist: Initialize hierachy tree explicitly

[Namhyung Kim <namhyung@kernel.org>]

On Mon, Sep 12, 2016 at 11:22:28AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Sep 12, 2016 at 03:19:55PM +0900, Namhyung Kim escreveu:
> > The hroot_in and hroot_out are root of hiearchy tree of hist entry.  But
> > as hist entry is initialized by copying existing template entry, it
> 
> Which makes default_ops->new() pointing to a zalloc based constructor
> totally useless, right? I.e. we malloc() + bzero() to then imediatelly
> memcpy from the template :-)

Right.

> 
> > sometimes has non-empty tree and copied it incorrectly.  This is a
> > problem especially when event group is used since it creates dummy
> > entries from already-processed entries in other event members.
> 
> I guess this needs a "Fixes:" probably for the changeset introducing
> those members :-\

This problem can occur only if event group is used with hierarchy
which will be enabled by this patchset.  In the normal hierarchy path,
hist entries are created before processing and have empty hierarchy
tree.  So current code has no problem.

Thanks,
Namhyung

> 
> - Arnaldo
>  
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/hist.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index 702ba3a8ead6..37a08f20730a 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -417,6 +417,8 @@ static int hist_entry__init(struct hist_entry *he,
> >  	}
> >  	INIT_LIST_HEAD(&he->pairs.node);
> >  	thread__get(he->thread);
> > +	he->hroot_in  = RB_ROOT;
> > +	he->hroot_out = RB_ROOT;
> >  
> >  	if (!symbol_conf.report_hierarchy)
> >  		he->leaf = true;
> > -- 
> > 2.9.3