Re: [PATCH] perf report: Fix invalid memory accessing

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Tue, Sep 08, 2015 at 01:13:59PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Sep 08, 2015 at 12:58:31PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Sep 08, 2015 at 12:49:10PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Tue, Sep 08, 2015 at 05:34:56PM +0200, Jiri Olsa escreveu:
> > > > On Tue, Sep 08, 2015 at 12:18:13PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > Humm, I think that we can have a pointer to the current perf_env, be it
> > > > > from the current machine, or from the machine environment in the
> > > > > perf.data file in struct machine, that way we don't need to change that
> > > > > function prototype, I'm prototyping this now, will post a patch.
> > > > 
> > > > I was thinking of that.. but the perf_env is actualyl related to the
> > > > perf.data not to the current machine.. I think it should be part of
> > > > the session or perf_header
> > > 
> > > But what if I want to trace only events that take place in some specific
> > > socket, i.e. what to do when perf_session is not used at all and we are
> > > not dealing with any header, since there are no files involved?
> > 
> > So, this is the continuation of this patch:
> > 
> > commit ce80d3bef9ff97638ca57a5659ef6ad356f35047
> > Author: Kan Liang <kan.liang@intel.com>
> > Date:   Fri Aug 28 05:48:04 2015 -0400
> > 
> >     perf tools: Rename perf_session_env to perf_env
> >     
> >     As it is not necessarily tied to a perf.data file and needs using in
> >     places where a perf_session is not required.
> >     
> >     Suggested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > -----------------------------
> > 
> > perf_env not necessarily is related to a perf.data file, we need even to
> > move it away from header.h.
> > 
> > I am looking now at where to populate perf_env and set it to
> > machine->env when no perf.data files are being accessed.
> > 
> > I  should have seen the use cpu_map__get_socket_id() in
> > perf_event__preprocess_sample(), that is unnaceptable, as it will parse
> > that file for each sample, right ;-\
> > 
> > Right now we don't have that much use for the other fields in
> > 'perf_env', just for the CPU topology information, that we will set in
> > addr_location for each sample, but we can have uses for that later,
> > think about a TUI interface for 'perf trace' where we will show what was
> > the command line, etc.
> 
> Argh, so in the patch introducing this al.socket thing it would first
> parse the value from the current system, reading sysfs, etc, then, in
> the 'report' case it would just throw this information away:
> 
> -       /* read socket id from perf.data for perf report */
> -       al.socket = env->cpu[al.cpu].socket_id;
> 
> We really should do this in perf_event__preprocess_sample() and read the
> topology information just once, probably using the same routine that
> creates the perf.data file env record.

Lunch break, but I'll continue the work I started at
https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=tmp.perf/core

- Arnaldo