Re: [PATCH V1 06/19] perf tools: Fix kernel_start for PTI on x86

[Arnaldo Carvalho de Melo]

Em Wed, May 16, 2018 at 09:16:57PM +0300, Adrian Hunter escreveu:
> On 05/16/2018 04:55 PM, Arnaldo Carvalho de Melo wrote:
> > Em Wed, May 16, 2018 at 04:45:41PM +0300, Adrian Hunter escreveu:
> >> On 16/05/18 16:00, Arnaldo Carvalho de Melo wrote:
> >>> 2303	bool machine__is(struct machine *machine, const char *arch)
> >>> 2304	{
> >>> 2305		return machine && machine->env && !strcmp(machine->env->arch, arch);
> >>> 2306	}
> >>> 2307	
> >>> 2308	int machine__get_kernel_start(struct machine *machine)
> >>> 2309	{
> >>> (gdb) p machine
> >>> $1 = (struct machine *) 0xc55548
> >>> (gdb) p machine->env
> >>> $2 = (struct perf_env *) 0xc06400 <perf_env>
> >>> (gdb) p machine-env->arch
> >>> No symbol "env" in current context.
> >>> (gdb) p machine->env->arch
> >>> $3 = 0x0
> >>> (gdb)
> >  
> >> If there is no perf_data then perf_session__new() uses perf_env but it seems
> >> perf_env.arch is not initialized.  Would it be OK to initialize
> >> perf_env.arch and perf_env.nr_cpus_avail?
> > 
> > I guess so, to make that always available, i.e. no perf.data? Those
> > should reflect the running machine environment.
> > 
> > So a preparatory patch that makes that the case would be good, I think.
> 
> How about this:
> 
> From: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> Date: Wed, 16 May 2018 19:58:19 +0300
> Subject: [PATCH] perf tools: Initialize perf_env.arch and
>  perf_env.nr_cpus_avail
> 
> Initialize perf_env.arch and perf_env.nr_cpus_avail so that they are
> available for tools to use, even for tools that are not processing a
> perf.data file.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> ---
>  tools/perf/perf.c     |  4 ++++
>  tools/perf/util/env.c | 36 ++++++++++++++++++++++++++++++++++++
>  tools/perf/util/env.h |  2 ++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/tools/perf/perf.c b/tools/perf/perf.c
> index 20a08cb32332..03ec7a3b996b 100644
> --- a/tools/perf/perf.c
> +++ b/tools/perf/perf.c
> @@ -300,7 +300,11 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>  	commit_pager_choice();
>  
>  	perf_env__set_cmdline(&perf_env, argc, argv);
> +	status = perf_env__read_common(&perf_env);
> +	if (status < 0)
> +		goto cleanup;


But this is being done unconditionally, right? We already do _tons_ of
stuff at startup, I think this should be done when we actually need it,
if not done before, i.e. either make tools that need it and know it is
not read because they don't read a perf.data file (perf-top) read it
explicitely or use perf_env__arch(machine->env) where one is not sure it
is already read, and that would do the read.

- Arnaldo