From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S932343AbbJ0O1I (ORCPT <rfc822;w@1wt.eu>);
	Tue, 27 Oct 2015 10:27:08 -0400
Received: from mail.kernel.org ([198.145.29.136]:53944 "EHLO mail.kernel.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S932219AbbJ0O1G (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Tue, 27 Oct 2015 10:27:06 -0400
Date: Tue, 27 Oct 2015 11:26:57 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Jiri Olsa <jolsa@kernel.org>
Cc: lkml <linux-kernel@vger.kernel.org>, David Ahern <dsahern@gmail.com>,
        Ingo Molnar <mingo@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>,
        "Liang, Kan" <kan.liang@intel.com>
Subject: Re: [PATCH 29/52] perf stat record: Add record command
Message-ID: <20151027142657.GD9405@kernel.org>
References: <1445784728-21732-1-git-send-email-jolsa@kernel.org>
 <1445784728-21732-30-git-send-email-jolsa@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1445784728-21732-30-git-send-email-jolsa@kernel.org>
X-Url: http://acmel.wordpress.com
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Em Sun, Oct 25, 2015 at 03:51:45PM +0100, Jiri Olsa escreveu:
> Add 'perf stat record' command support. It creates simple
> (header only) perf.data file ATM.

Huh? Couldn't we have the tools providing a sensible message at this point?

[root@zoo linux]# rm -f perf.data
[root@zoo linux]# perf stat record usleep 1

 Performance counter stats for 'usleep 1':

          0.575678      task-clock (msec)         #    0.508 CPUs utilized          
                 1      context-switches          #    0.002 M/sec                  
                 0      cpu-migrations            #    0.000 K/sec                  
                52      page-faults               #    0.090 M/sec                  
            905601      cycles                    #    1.573 GHz                    
            608259      stalled-cycles-frontend   #   67.17% frontend cycles idle   
   <not supported>      stalled-cycles-backend   
            616889      instructions              #    0.68  insns per cycle        
                                                  #    0.99  stalled cycles per insn
            125710      branches                  #  218.369 M/sec                  
              7589      branch-misses             #    6.04% of all branches        

       0.001134076 seconds time elapsed

[root@zoo linux]# perf evlist
WARNING: The perf.data file's data size field is 0 which is unexpected.
Was the 'perf record' command properly terminated?
non matching sample_type[root@zoo linux]# 
[root@zoo linux]# perf report --stdio
WARNING: The perf.data file's data size field is 0 which is unexpected.
Was the 'perf record' command properly terminated?
non matching sample_type[root@zoo linux]#

 ---------------------------------------------------------------------------------------

Can't we make it so that older tools will juts state that there are no samples, i.e.:

[root@zoo linux]# perf record -e alignment-faults usleep 1
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.018 MB perf.data ]
[root@zoo linux]# perf evlist
alignment-faults
[root@zoo linux]# perf report --stdio
Error:
The perf.data file has no samples!
# To display the perf.data header info, please use --header/--header-only options.
#
[root@zoo linux]#

--------------------------------------------------------------------------------

This looks a friendlier message, as the 'perf stat record' file really has no
samples, by definition :-)

This is not a show stopper tho, to make progress, I'll continue processing the
patches here, but please consider changing this patch to improve the output
produced by older tools.

- Arnaldo
 
> Link: http://lkml.kernel.org/n/tip-0av5yfkwyywwgoiali88w4hi@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/Documentation/perf-stat.txt | 12 ++++++
>  tools/perf/builtin-stat.c              | 74 +++++++++++++++++++++++++++++++++-
>  2 files changed, 84 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
> index 4e074a660826..70eee1c2c444 100644
> --- a/tools/perf/Documentation/perf-stat.txt
> +++ b/tools/perf/Documentation/perf-stat.txt
> @@ -10,6 +10,7 @@ SYNOPSIS
>  [verse]
>  'perf stat' [-e <EVENT> | --event=EVENT] [-a] <command>
>  'perf stat' [-e <EVENT> | --event=EVENT] [-a] -- <command> [<options>]
> +'perf stat' [-e <EVENT> | --event=EVENT] [-a] record [-o file] -- <command> [<options>]
>  
>  DESCRIPTION
>  -----------
> @@ -22,6 +23,8 @@ OPTIONS
>  <command>...::
>  	Any command you can specify in a shell.
>  
> +record::
> +	See STAT RECORD.
>  
>  -e::
>  --event=::
> @@ -159,6 +162,15 @@ filter out the startup phase of the program, which is often very different.
>  
>  Print statistics of transactional execution if supported.
>  
> +STAT RECORD
> +-----------
> +Stores stat data into perf data file.
> +
> +-o file::
> +--output file::
> +Output file name.
> +
> +
>  EXAMPLES
>  --------
>  
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index a3880aa65b04..b65cc6a46226 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -59,6 +59,7 @@
>  #include "util/thread.h"
>  #include "util/thread_map.h"
>  #include "util/counts.h"
> +#include "util/session.h"
>  
>  #include <stdlib.h>
>  #include <sys/prctl.h>
> @@ -123,6 +124,16 @@ static struct timespec		ref_time;
>  static struct cpu_map		*aggr_map;
>  static aggr_get_id_t		aggr_get_id;
>  
> +struct perf_stat {
> +	bool			 record;
> +	struct perf_data_file	 file;
> +	struct perf_session	*session;
> +	u64			 bytes_written;
> +};
> +
> +static struct perf_stat		perf_stat;
> +#define STAT_RECORD		perf_stat.record
> +
>  static volatile int done = 0;
>  
>  static struct perf_stat_config stat_config = {
> @@ -341,6 +352,15 @@ static int __run_perf_stat(int argc, const char **argv)
>  		return -1;
>  	}
>  
> +	if (STAT_RECORD) {
> +		int err, fd = perf_data_file__fd(&perf_stat.file);
> +
> +		err = perf_session__write_header(perf_stat.session, evsel_list,
> +						 fd, false);
> +		if (err < 0)
> +			return err;
> +	}
> +
>  	/*
>  	 * Enable counters and exec the command:
>  	 */
> @@ -1192,6 +1212,39 @@ static int add_default_attributes(void)
>  	return perf_evlist__add_default_attrs(evsel_list, very_very_detailed_attrs);
>  }
>  
> +static const char * const recort_usage[] = {
> +	"perf stat record [<options>]",
> +	NULL,
> +};
> +
> +static int __cmd_record(int argc, const char **argv)
> +{
> +	struct perf_session *session;
> +	struct perf_data_file *file = &perf_stat.file;
> +	const struct option options[] = {
> +	OPT_STRING('o', "output", &perf_stat.file.path, "file", "output file name"),
> +	OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, options, record_usage,
> +			     PARSE_OPT_STOP_AT_NON_OPTION);
> +
> +	session = perf_session__new(file, false, NULL);
> +	if (session == NULL) {
> +		pr_err("Perf session creation failed.\n");
> +		return -1;
> +	}
> +
> +	/* No pipe support ATM */
> +	if (perf_stat.file.is_pipe)
> +		return -EINVAL;
> +
> +	session->evlist   = evsel_list;
> +	perf_stat.session = session;
> +	perf_stat.record  = true;
> +	return argc;
> +}
> +
>  int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
>  {
>  	bool append_file = false;
> @@ -1265,6 +1318,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
>  	const char *mode;
>  	FILE *output = stderr;
>  	unsigned int interval;
> +	const char * const stat_subcommands[] = { "record" };
>  
>  	setlocale(LC_ALL, "");
>  
> @@ -1272,8 +1326,15 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
>  	if (evsel_list == NULL)
>  		return -ENOMEM;
>  
> -	argc = parse_options(argc, argv, options, stat_usage,
> -		PARSE_OPT_STOP_AT_NON_OPTION);
> +	argc = parse_options_subcommand(argc, argv, options, stat_subcommands,
> +					(const char **) stat_usage,
> +					PARSE_OPT_STOP_AT_NON_OPTION);
> +
> +	if (argc && !strncmp(argv[0], "rec", 3)) {
> +		argc = __cmd_record(argc, argv);
> +		if (argc < 0)
> +			return -1;
> +	}
>  
>  	interval = stat_config.interval;
>  
> @@ -1444,6 +1505,15 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
>  	if (!forever && status != -1 && !interval)
>  		print_counters(NULL, argc, argv);
>  
> +	if (STAT_RECORD) {
> +		int fd = perf_data_file__fd(&perf_stat.file);
> +
> +		perf_stat.session->header.data_size += perf_stat.bytes_written;
> +		perf_session__write_header(perf_stat.session, evsel_list, fd, true);
> +
> +		perf_session__delete(perf_stat.session);
> +	}
> +
>  	perf_evlist__free_stats(evsel_list);
>  out:
>  	perf_evlist__delete(evsel_list);
> -- 
> 2.4.3