Re: [PATCH 10/16] perf tools: Add support for synthesized sample type

From: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
To: Adrian Hunter <adrian.hunter@intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>, Andi Kleen <ak@linux.intel.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 10/16] perf tools: Add support for synthesized sample type
Date: Thu, 16 Apr 2020 12:01:06 -0300	[thread overview]
Message-ID: <20200416150106.GB31666@kernel.org> (raw)
In-Reply-To: <20200416145704.GA1842@kernel.org>

Em Thu, Apr 16, 2020 at 11:57:04AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Apr 16, 2020 at 11:54:26AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Apr 01, 2020 at 01:16:07PM +0300, Adrian Hunter escreveu:
> > > +++ b/tools/perf/util/evsel.c
> > > @@ -2136,7 +2136,7 @@ int perf_evsel__parse_sample(struct evsel *evsel, union perf_event *event,
> > >  		}
> > >  	}

> > > -	if (evsel__has_callchain(evsel)) {
> > > +	if (type & PERF_SAMPLE_CALLCHAIN) {
> > >  		const u64 max_callchain_nr = UINT64_MAX / sizeof(u64);

> > This ends up looking unrelated, I had to go and look at the source to
> > see that this is just a simplification, i.e. earlier in this function
> > (perf_evsel__parse_sample) we have:

> >         u64 type = evsel->core.attr.sample_type;

> > So the above doesn't change anything, good, but slowed reviewing a bit,
> > please consider next time to have this in a separate patch, I'll do it
> > this time.

> I've added this as the cset comment:

> Using 'type' variable for checking for callchains is equivalent to using
> evsel__has_callchain(evsel) and is how the other PERF_SAMPLE_ bits are checked
> in this function, so use it to be consistent.

Then, reading the rest of the patch, evsel__has_callchain() is not
anymore equivalent to just looking at the PERF_SAMPLE_ bit in core.attr
:-)

- Arnaldo