[PATCH RFC 0/2] perf record: Put a copy of kcore into the perf.data directory

[PATCH RFC 0/2] perf record: Put a copy of kcore into the perf.data directory

[Adrian Hunter]

Hi

Here are 2 patches to add an option to 'perf record' to put a copy of kcore
into a directory with perf.data.  Refer to patch 2 for an example.


Adrian Hunter (2):
      perf tools: Support single perf.data file directory
      perf record: Put a copy of kcore into the perf.data directory

 tools/perf/Documentation/perf-record.txt |  3 ++
 tools/perf/builtin-record.c              | 54 +++++++++++++++++++++++-
 tools/perf/util/data.c                   | 70 +++++++++++++++++++++++++++-----
 tools/perf/util/data.h                   |  8 ++++
 tools/perf/util/header.h                 |  1 +
 tools/perf/util/record.h                 |  1 +
 tools/perf/util/session.c                |  6 ++-
 tools/perf/util/util.c                   | 18 ++++++++
 8 files changed, 149 insertions(+), 12 deletions(-)


Regards
Adrian

[PATCH RFC 1/2] perf tools: Support single perf.data file directory

[Adrian Hunter]

Support directory output that contains a regular perf.data file. This is
preparation for adding support for putting a copy of /proc/kcore in that
directory.

Distinguish the multiple file case from the regular (single) perf.data file
case by adding data->is_multi_file.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/builtin-record.c |  2 +-
 tools/perf/util/data.c      | 37 +++++++++++++++++++++++++++----------
 tools/perf/util/data.h      |  6 ++++++
 tools/perf/util/header.h    |  1 +
 tools/perf/util/session.c   |  2 +-
 tools/perf/util/util.c      |  1 +
 6 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 1447004eee8a..4c356a046dd3 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -534,7 +534,7 @@ static int record__process_auxtrace(struct perf_tool *tool,
 	size_t padding;
 	u8 pad[8] = {0};
 
-	if (!perf_data__is_pipe(data) && !perf_data__is_dir(data)) {
+	if (!perf_data__is_pipe(data) && !perf_data__is_multi_file(data)) {
 		off_t file_offset;
 		int fd = perf_data__fd(data);
 		int err;
diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index 88fba2ba549f..2fd3114d1167 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -37,7 +37,7 @@ int perf_data__create_dir(struct perf_data *data, int nr)
 	struct perf_data_file *files = NULL;
 	int i, ret = -1;
 
-	if (WARN_ON(!data->is_dir))
+	if (WARN_ON(!data->is_multi_file))
 		return -EINVAL;
 
 	files = zalloc(nr * sizeof(*files));
@@ -76,7 +76,7 @@ int perf_data__open_dir(struct perf_data *data)
 	DIR *dir;
 	int nr = 0;
 
-	if (WARN_ON(!data->is_dir))
+	if (WARN_ON(!data->is_multi_file))
 		return -EINVAL;
 
 	/* The version is provided by DIR_FORMAT feature. */
@@ -217,6 +217,16 @@ static bool is_dir(struct perf_data *data)
 	return (st.st_mode & S_IFMT) == S_IFDIR;
 }
 
+static bool is_multi_file(struct perf_data *data)
+{
+	char path[PATH_MAX];
+	struct stat st;
+
+	snprintf(path, sizeof(path), "%s/header", data->path);
+
+	return !stat(path, &st);
+}
+
 static int open_file_read(struct perf_data *data)
 {
 	struct stat st;
@@ -302,12 +312,17 @@ static int open_dir(struct perf_data *data)
 {
 	int ret;
 
-	/*
-	 * So far we open only the header, so we can read the data version and
-	 * layout.
-	 */
-	if (asprintf(&data->file.path, "%s/header", data->path) < 0)
-		return -1;
+	if (perf_data__is_multi_file(data)) {
+		/*
+		 * So far we open only the header, so we can read the data version and
+		 * layout.
+		 */
+		if (asprintf(&data->file.path, "%s/header", data->path) < 0)
+			return -1;
+	} else {
+		if (asprintf(&data->file.path, "%s/perf.data", data->path) < 0)
+			return -1;
+	}
 
 	if (perf_data__is_write(data) &&
 	    mkdir(data->path, S_IRWXU) < 0)
@@ -333,8 +348,10 @@ int perf_data__open(struct perf_data *data)
 	if (check_backup(data))
 		return -1;
 
-	if (perf_data__is_read(data))
+	if (perf_data__is_read(data)) {
 		data->is_dir = is_dir(data);
+		data->is_multi_file = is_multi_file(data);
+	}
 
 	return perf_data__is_dir(data) ?
 	       open_dir(data) : open_file_dup(data);
@@ -406,7 +423,7 @@ unsigned long perf_data__size(struct perf_data *data)
 	u64 size = data->file.size;
 	int i;
 
-	if (!data->is_dir)
+	if (!data->is_multi_file)
 		return size;
 
 	for (i = 0; i < data->dir.nr; i++) {
diff --git a/tools/perf/util/data.h b/tools/perf/util/data.h
index 259868a39019..0c5994d969c7 100644
--- a/tools/perf/util/data.h
+++ b/tools/perf/util/data.h
@@ -20,6 +20,7 @@ struct perf_data {
 	struct perf_data_file	 file;
 	bool			 is_pipe;
 	bool			 is_dir;
+	bool			 is_multi_file;
 	bool			 force;
 	enum perf_data_mode	 mode;
 
@@ -50,6 +51,11 @@ static inline bool perf_data__is_dir(struct perf_data *data)
 	return data->is_dir;
 }
 
+static inline bool perf_data__is_multi_file(struct perf_data *data)
+{
+	return data->is_multi_file;
+}
+
 static inline int perf_data__fd(struct perf_data *data)
 {
 	return data->file.fd;
diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index 999dac41871e..73ad217958b8 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -53,6 +53,7 @@ enum perf_header_version {
 };
 
 enum perf_dir_version {
+	PERF_DIR_SINGLE_FILE	= 0,
 	PERF_DIR_VERSION	= 1,
 };
 
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 2b583e6adb49..6dd04bd092d1 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -222,7 +222,7 @@ struct perf_session *perf_session__new(struct perf_data *data,
 			perf_evlist__init_trace_event_sample_raw(session->evlist);
 
 			/* Open the directory data. */
-			if (data->is_dir && perf_data__open_dir(data))
+			if (data->is_multi_file && perf_data__open_dir(data))
 				goto out_delete;
 		}
 	} else  {
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 32322a20a68b..5ca7490afdd1 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -189,6 +189,7 @@ int rm_rf_perf_data(const char *path)
 	const char *pat[] = {
 		"header",
 		"data.*",
+		"perf.data",
 		NULL,
 	};
 
-- 
2.17.1

[PATCH RFC 2/2] perf record: Put a copy of kcore into the perf.data directory

[Adrian Hunter]

Add a new 'perf record' option '--kcore' which will put a copy of /proc/kcore,
kallsyms and modules into the perf.data directory.

Example:

$ sudo perf record --kcore uname

$ sudo tree perf.data
perf.data
├── kcore_dir
│   ├── kallsyms
│   ├── kcore
│   └── modules
└── perf.data

$ sudo perf script -v
build id event received for vmlinux: 1eaa285996affce2d74d8e66dcea09a80c9941de
build id event received for [vdso]: 8bbaf5dc62a9b644b4d4e4539737e104e4a84541
Samples for 'cycles' event do not have CPU attribute set. Skipping 'cpu' field.
Using CPUID GenuineIntel-6-8E-A
Using perf.data/kcore_dir/kcore for kernel data
Using perf.data/kcore_dir/kallsyms for symbols
            perf 19058 506778.423729:          1 cycles:  ffffffffa2caa548 native_write_msr+0x8 (vmlinux)
            perf 19058 506778.423733:          1 cycles:  ffffffffa2caa548 native_write_msr+0x8 (vmlinux)
            perf 19058 506778.423734:          7 cycles:  ffffffffa2caa548 native_write_msr+0x8 (vmlinux)
            perf 19058 506778.423736:        117 cycles:  ffffffffa2caa54a native_write_msr+0xa (vmlinux)
            perf 19058 506778.423738:       2092 cycles:  ffffffffa2c9b7b0 native_apic_msr_write+0x0 (vmlinux)
            perf 19058 506778.423740:      37380 cycles:  ffffffffa2f121d0 perf_event_addr_filters_exec+0x0 (vmlinux)
           uname 19058 506778.423751:     582673 cycles:  ffffffffa303a407 propagate_protected_usage+0x147 (vmlinux)
           uname 19058 506778.423892:    2241841 cycles:  ffffffffa2cae0c9 unwind_next_frame.part.5+0x79 (vmlinux)
           uname 19058 506778.424430:    2457397 cycles:  ffffffffa3019232 check_memory_region+0x52 (vmlinux)

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/Documentation/perf-record.txt |  3 ++
 tools/perf/builtin-record.c              | 52 ++++++++++++++++++++++++
 tools/perf/util/data.c                   | 33 +++++++++++++++
 tools/perf/util/data.h                   |  2 +
 tools/perf/util/record.h                 |  1 +
 tools/perf/util/session.c                |  4 ++
 tools/perf/util/util.c                   | 17 ++++++++
 7 files changed, 112 insertions(+)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index c6f9f31b6039..1d85e24aa453 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -571,6 +571,9 @@ config terms. For example: 'cycles/overwrite/' and 'instructions/no-overwrite/'.
 
 Implies --tail-synthesize.
 
+--kcore::
+Make a copy of /proc/kcore and place it into a directory with the perf.data file.
+
 SEE ALSO
 --------
 linkperf:perf-stat[1], linkperf:perf-list[1]
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 4c356a046dd3..b665ec7254b9 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -53,6 +53,9 @@
 #include <signal.h>
 #include <sys/mman.h>
 #include <sys/wait.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
 #include <linux/string.h>
 #include <linux/time64.h>
 #include <linux/zalloc.h>
@@ -696,6 +699,37 @@ static int record__auxtrace_init(struct record *rec __maybe_unused)
 
 #endif
 
+static bool record__kcore_readable(struct machine *machine)
+{
+	char kcore[PATH_MAX];
+	int fd;
+
+	scnprintf(kcore, sizeof(kcore), "%s/proc/kcore", machine->root_dir);
+
+	fd = open(kcore, O_RDONLY);
+	if (fd < 0)
+		return false;
+
+	close(fd);
+
+	return true;
+}
+
+static int record__kcore_copy(struct machine *machine, struct perf_data *data)
+{
+	char from_dir[PATH_MAX];
+	char kcore_dir[PATH_MAX];
+	int ret;
+
+	snprintf(from_dir, sizeof(from_dir), "%s/proc", machine->root_dir);
+
+	ret = perf_data__make_kcore_dir(data, kcore_dir, sizeof(kcore_dir));
+	if (ret)
+		return ret;
+
+	return kcore_copy(from_dir, kcore_dir);
+}
+
 static int record__mmap_evlist(struct record *rec,
 			       struct evlist *evlist)
 {
@@ -1378,6 +1412,12 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	session->header.env.comp_type  = PERF_COMP_ZSTD;
 	session->header.env.comp_level = rec->opts.comp_level;
 
+	if (rec->opts.kcore &&
+	    !record__kcore_readable(&session->machines.host)) {
+		pr_err("ERROR: kcore is not readable.\n");
+		return -1;
+	}
+
 	record__init_features(rec);
 
 	if (rec->opts.use_clockid && rec->opts.clockid_res_ns)
@@ -1409,6 +1449,14 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	}
 	session->header.env.comp_mmap_len = session->evlist->mmap_len;
 
+	if (rec->opts.kcore) {
+		err = record__kcore_copy(&session->machines.host, data);
+		if (err) {
+			pr_err("ERROR: Failed to copy kcore\n");
+			goto out_child;
+		}
+	}
+
 	err = bpf__apply_obj_config();
 	if (err) {
 		char errbuf[BUFSIZ];
@@ -2179,6 +2227,7 @@ static struct option __record_options[] = {
 		     parse_cgroups),
 	OPT_UINTEGER('D', "delay", &record.opts.initial_delay,
 		  "ms to wait before starting measurement after program start"),
+	OPT_BOOLEAN(0, "kcore", &record.opts.kcore, "copy /proc/kcore"),
 	OPT_STRING('u', "uid", &record.opts.target.uid_str, "user",
 		   "user to profile"),
 
@@ -2317,6 +2366,9 @@ int cmd_record(int argc, const char **argv)
 
 	}
 
+	if (rec->opts.kcore)
+		rec->data.is_dir = true;
+
 	if (rec->opts.comp_level != 0) {
 		pr_debug("Compression enabled, disabling build id collection at the end of the session.\n");
 		rec->no_buildid = true;
diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index 2fd3114d1167..86b2d6cf5f11 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -434,3 +434,36 @@ unsigned long perf_data__size(struct perf_data *data)
 
 	return size;
 }
+
+int perf_data__make_kcore_dir(struct perf_data *data, char *buf, size_t buf_sz)
+{
+	int ret;
+
+	if (!data->is_dir)
+		return -1;
+
+	ret = snprintf(buf, buf_sz, "%s/kcore_dir", data->path);
+	if (ret < 0 || (size_t)ret >= buf_sz)
+		return -1;
+
+	return mkdir(buf, S_IRWXU);
+}
+
+char *perf_data__kallsyms_name(struct perf_data *data)
+{
+	char *kallsyms_name;
+	struct stat st;
+
+	if (!data->is_dir)
+		return NULL;
+
+	if (asprintf(&kallsyms_name, "%s/kcore_dir/kallsyms", data->path) < 0)
+		return NULL;
+
+	if (stat(kallsyms_name, &st)) {
+		free(kallsyms_name);
+		return NULL;
+	}
+
+	return kallsyms_name;
+}
diff --git a/tools/perf/util/data.h b/tools/perf/util/data.h
index 0c5994d969c7..cb906b524897 100644
--- a/tools/perf/util/data.h
+++ b/tools/perf/util/data.h
@@ -83,4 +83,6 @@ int perf_data__open_dir(struct perf_data *data);
 void perf_data__close_dir(struct perf_data *data);
 int perf_data__update_dir(struct perf_data *data);
 unsigned long perf_data__size(struct perf_data *data);
+int perf_data__make_kcore_dir(struct perf_data *data, char *buf, size_t buf_sz);
+char *perf_data__kallsyms_name(struct perf_data *data);
 #endif /* __PERF_DATA_H */
diff --git a/tools/perf/util/record.h b/tools/perf/util/record.h
index 00275afc524d..948bbcf9aef3 100644
--- a/tools/perf/util/record.h
+++ b/tools/perf/util/record.h
@@ -44,6 +44,7 @@ struct record_opts {
 	bool	      strict_freq;
 	bool	      sample_id;
 	bool	      no_bpf_event;
+	bool	      kcore;
 	unsigned int  freq;
 	unsigned int  mmap_pages;
 	unsigned int  auxtrace_mmap_pages;
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 6dd04bd092d1..00d3e75ca1f4 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -224,6 +224,10 @@ struct perf_session *perf_session__new(struct perf_data *data,
 			/* Open the directory data. */
 			if (data->is_multi_file && perf_data__open_dir(data))
 				goto out_delete;
+
+			if (!symbol_conf.kallsyms_name &&
+			    !symbol_conf.vmlinux_name)
+				symbol_conf.kallsyms_name = perf_data__kallsyms_name(data);
 		}
 	} else  {
 		session->machines.host.env = &perf_env;
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 5ca7490afdd1..44568ce5082f 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -184,6 +184,21 @@ static int rm_rf_depth_pat(const char *path, int depth, const char **pat)
 	return rmdir(path);
 }
 
+static int rm_rf_kcore_dir(const char *path)
+{
+	char kcore_dir_path[PATH_MAX];
+	const char *pat[] = {
+		"kcore",
+		"kallsyms",
+		"modules",
+		NULL,
+	};
+
+	snprintf(kcore_dir_path, sizeof(kcore_dir_path), "%s/kcore_dir", path);
+
+	return rm_rf_depth_pat(kcore_dir_path, 0, pat);
+}
+
 int rm_rf_perf_data(const char *path)
 {
 	const char *pat[] = {
@@ -193,6 +208,8 @@ int rm_rf_perf_data(const char *path)
 		NULL,
 	};
 
+	rm_rf_kcore_dir(path);
+
 	return rm_rf_depth_pat(path, 0, pat);
 }
 
-- 
2.17.1

Re: [PATCH RFC 1/2] perf tools: Support single perf.data file directory

[Jiri Olsa]

On Mon, Sep 16, 2019 at 11:56:45AM +0300, Adrian Hunter wrote:
> Support directory output that contains a regular perf.data file. This is
> preparation for adding support for putting a copy of /proc/kcore in that
> directory.
> 
> Distinguish the multiple file case from the regular (single) perf.data file
> case by adding data->is_multi_file.

SNIP

>  static int open_file_read(struct perf_data *data)
>  {
>  	struct stat st;
> @@ -302,12 +312,17 @@ static int open_dir(struct perf_data *data)
>  {
>  	int ret;
>  
> -	/*
> -	 * So far we open only the header, so we can read the data version and
> -	 * layout.
> -	 */
> -	if (asprintf(&data->file.path, "%s/header", data->path) < 0)
> -		return -1;
> +	if (perf_data__is_multi_file(data)) {
> +		/*
> +		 * So far we open only the header, so we can read the data version and
> +		 * layout.
> +		 */
> +		if (asprintf(&data->file.path, "%s/header", data->path) < 0)
> +			return -1;
> +	} else {
> +		if (asprintf(&data->file.path, "%s/perf.data", data->path) < 0)
> +			return -1;
> +	}

first, please note that there's support for perf.data directory code,
but it's not been enabled yet, so we can do any changes there without
breaking existing users

currently the logic is prepared to have perf.data DIR_FORMAT feature
to define the layout of the directory

it'd be great to have just single point where we get directory layout,
not checking on files names first and checking on DIR_FORMAT later

also the kcore will be beneficial for other layouts,
so would be great to make it somehow optional/switchable

one of the options could be to have DIR_FORMAT feature as the source
of directory layout and it'd have bitmask of files/dirs (like kcore_dir)
available in the directory

thanks,
jirka

Re: [PATCH RFC 1/2] perf tools: Support single perf.data file directory

[Adrian Hunter]

On 24/09/19 12:34 AM, Jiri Olsa wrote:
> On Mon, Sep 16, 2019 at 11:56:45AM +0300, Adrian Hunter wrote:
>> Support directory output that contains a regular perf.data file. This is
>> preparation for adding support for putting a copy of /proc/kcore in that
>> directory.
>>
>> Distinguish the multiple file case from the regular (single) perf.data file
>> case by adding data->is_multi_file.
> 
> SNIP
> 
>>  static int open_file_read(struct perf_data *data)
>>  {
>>  	struct stat st;
>> @@ -302,12 +312,17 @@ static int open_dir(struct perf_data *data)
>>  {
>>  	int ret;
>>  
>> -	/*
>> -	 * So far we open only the header, so we can read the data version and
>> -	 * layout.
>> -	 */
>> -	if (asprintf(&data->file.path, "%s/header", data->path) < 0)
>> -		return -1;
>> +	if (perf_data__is_multi_file(data)) {
>> +		/*
>> +		 * So far we open only the header, so we can read the data version and
>> +		 * layout.
>> +		 */
>> +		if (asprintf(&data->file.path, "%s/header", data->path) < 0)
>> +			return -1;
>> +	} else {
>> +		if (asprintf(&data->file.path, "%s/perf.data", data->path) < 0)
>> +			return -1;
>> +	}
> 

Thanks for replying :-)

> first, please note that there's support for perf.data directory code,
> but it's not been enabled yet, so we can do any changes there without
> breaking existing users
> 
> currently the logic is prepared to have perf.data DIR_FORMAT feature
> to define the layout of the directory
> 
> it'd be great to have just single point where we get directory layout,
> not checking on files names first and checking on DIR_FORMAT later

Ok, but what are you suggesting?  Naming the data file "header" seems a bit
counter-intuitive in this case.

> 
> also the kcore will be beneficial for other layouts,
> so would be great to make it somehow optional/switchable

In these patches it is, because it is not related to the DIR_FORMAT.

> one of the options could be to have DIR_FORMAT feature as the source
> of directory layout and it'd have bitmask of files/dirs (like kcore_dir)
> available in the directory

Is there an advantage to making optional files/dirs part of the format?
i.e. if they are there, use them otherwise don't.

Re: [PATCH RFC 1/2] perf tools: Support single perf.data file directory

[Jiri Olsa]

On Tue, Sep 24, 2019 at 12:12:25PM +0300, Adrian Hunter wrote:
> On 24/09/19 12:34 AM, Jiri Olsa wrote:
> > On Mon, Sep 16, 2019 at 11:56:45AM +0300, Adrian Hunter wrote:
> >> Support directory output that contains a regular perf.data file. This is
> >> preparation for adding support for putting a copy of /proc/kcore in that
> >> directory.
> >>
> >> Distinguish the multiple file case from the regular (single) perf.data file
> >> case by adding data->is_multi_file.
> > 
> > SNIP
> > 
> >>  static int open_file_read(struct perf_data *data)
> >>  {
> >>  	struct stat st;
> >> @@ -302,12 +312,17 @@ static int open_dir(struct perf_data *data)
> >>  {
> >>  	int ret;
> >>  
> >> -	/*
> >> -	 * So far we open only the header, so we can read the data version and
> >> -	 * layout.
> >> -	 */
> >> -	if (asprintf(&data->file.path, "%s/header", data->path) < 0)
> >> -		return -1;
> >> +	if (perf_data__is_multi_file(data)) {
> >> +		/*
> >> +		 * So far we open only the header, so we can read the data version and
> >> +		 * layout.
> >> +		 */
> >> +		if (asprintf(&data->file.path, "%s/header", data->path) < 0)
> >> +			return -1;
> >> +	} else {
> >> +		if (asprintf(&data->file.path, "%s/perf.data", data->path) < 0)
> >> +			return -1;
> >> +	}
> > 
> 
> Thanks for replying :-)
> 
> > first, please note that there's support for perf.data directory code,
> > but it's not been enabled yet, so we can do any changes there without
> > breaking existing users
> > 
> > currently the logic is prepared to have perf.data DIR_FORMAT feature
> > to define the layout of the directory
> > 
> > it'd be great to have just single point where we get directory layout,
> > not checking on files names first and checking on DIR_FORMAT later
> 
> Ok, but what are you suggesting?  Naming the data file "header" seems a bit
> counter-intuitive in this case.

don't know ;-)

but I'd like to have one way of finding out the directory layout

the code for threaded record uses DIR_FORMAT feature value
to ensure the directory contains the expected files, which
is data file with 'data.<cpu>' name for every cpu

> 
> > 
> > also the kcore will be beneficial for other layouts,
> > so would be great to make it somehow optional/switchable
> 
> In these patches it is, because it is not related to the DIR_FORMAT.
> 
> > one of the options could be to have DIR_FORMAT feature as the source
> > of directory layout and it'd have bitmask of files/dirs (like kcore_dir)
> > available in the directory
> 
> Is there an advantage to making optional files/dirs part of the format?
> i.e. if they are there, use them otherwise don't.

ok, that might work, but please make that somehow explicit/visible
what files/directories are possible in the directory, so we could
easily see them and add new ones

jirka

Re: [PATCH RFC 1/2] perf tools: Support single perf.data file directory

[Adrian Hunter]

On 24/09/19 2:12 PM, Jiri Olsa wrote:
> On Tue, Sep 24, 2019 at 12:12:25PM +0300, Adrian Hunter wrote:
>> On 24/09/19 12:34 AM, Jiri Olsa wrote:
>>> On Mon, Sep 16, 2019 at 11:56:45AM +0300, Adrian Hunter wrote:
>>>> Support directory output that contains a regular perf.data file. This is
>>>> preparation for adding support for putting a copy of /proc/kcore in that
>>>> directory.
>>>>
>>>> Distinguish the multiple file case from the regular (single) perf.data file
>>>> case by adding data->is_multi_file.
>>>
>>> SNIP
>>>
>>>>  static int open_file_read(struct perf_data *data)
>>>>  {
>>>>  	struct stat st;
>>>> @@ -302,12 +312,17 @@ static int open_dir(struct perf_data *data)
>>>>  {
>>>>  	int ret;
>>>>  
>>>> -	/*
>>>> -	 * So far we open only the header, so we can read the data version and
>>>> -	 * layout.
>>>> -	 */
>>>> -	if (asprintf(&data->file.path, "%s/header", data->path) < 0)
>>>> -		return -1;
>>>> +	if (perf_data__is_multi_file(data)) {
>>>> +		/*
>>>> +		 * So far we open only the header, so we can read the data version and
>>>> +		 * layout.
>>>> +		 */
>>>> +		if (asprintf(&data->file.path, "%s/header", data->path) < 0)
>>>> +			return -1;
>>>> +	} else {
>>>> +		if (asprintf(&data->file.path, "%s/perf.data", data->path) < 0)
>>>> +			return -1;
>>>> +	}
>>>
>>
>> Thanks for replying :-)
>>
>>> first, please note that there's support for perf.data directory code,
>>> but it's not been enabled yet, so we can do any changes there without
>>> breaking existing users
>>>
>>> currently the logic is prepared to have perf.data DIR_FORMAT feature
>>> to define the layout of the directory
>>>
>>> it'd be great to have just single point where we get directory layout,
>>> not checking on files names first and checking on DIR_FORMAT later
>>
>> Ok, but what are you suggesting?  Naming the data file "header" seems a bit
>> counter-intuitive in this case.
> 
> don't know ;-)

So what about calling it "data" instead of "header"?

> 
> but I'd like to have one way of finding out the directory layout
> 
> the code for threaded record uses DIR_FORMAT feature value
> to ensure the directory contains the expected files, which
> is data file with 'data.<cpu>' name for every cpu
> 
>>
>>>
>>> also the kcore will be beneficial for other layouts,
>>> so would be great to make it somehow optional/switchable
>>
>> In these patches it is, because it is not related to the DIR_FORMAT.
>>
>>> one of the options could be to have DIR_FORMAT feature as the source
>>> of directory layout and it'd have bitmask of files/dirs (like kcore_dir)
>>> available in the directory
>>
>> Is there an advantage to making optional files/dirs part of the format?
>> i.e. if they are there, use them otherwise don't.
> 
> ok, that might work, but please make that somehow explicit/visible
> what files/directories are possible in the directory, so we could
> easily see them and add new ones

At the moment, what can exist is what can be removed i.e. see
rm_rf_perf_data().  Will that do?

Re: [PATCH RFC 1/2] perf tools: Support single perf.data file directory

[Jiri Olsa]

On Tue, Sep 24, 2019 at 02:51:28PM +0300, Adrian Hunter wrote:
> On 24/09/19 2:12 PM, Jiri Olsa wrote:
> > On Tue, Sep 24, 2019 at 12:12:25PM +0300, Adrian Hunter wrote:
> >> On 24/09/19 12:34 AM, Jiri Olsa wrote:
> >>> On Mon, Sep 16, 2019 at 11:56:45AM +0300, Adrian Hunter wrote:
> >>>> Support directory output that contains a regular perf.data file. This is
> >>>> preparation for adding support for putting a copy of /proc/kcore in that
> >>>> directory.
> >>>>
> >>>> Distinguish the multiple file case from the regular (single) perf.data file
> >>>> case by adding data->is_multi_file.
> >>>
> >>> SNIP
> >>>
> >>>>  static int open_file_read(struct perf_data *data)
> >>>>  {
> >>>>  	struct stat st;
> >>>> @@ -302,12 +312,17 @@ static int open_dir(struct perf_data *data)
> >>>>  {
> >>>>  	int ret;
> >>>>  
> >>>> -	/*
> >>>> -	 * So far we open only the header, so we can read the data version and
> >>>> -	 * layout.
> >>>> -	 */
> >>>> -	if (asprintf(&data->file.path, "%s/header", data->path) < 0)
> >>>> -		return -1;
> >>>> +	if (perf_data__is_multi_file(data)) {
> >>>> +		/*
> >>>> +		 * So far we open only the header, so we can read the data version and
> >>>> +		 * layout.
> >>>> +		 */
> >>>> +		if (asprintf(&data->file.path, "%s/header", data->path) < 0)
> >>>> +			return -1;
> >>>> +	} else {
> >>>> +		if (asprintf(&data->file.path, "%s/perf.data", data->path) < 0)
> >>>> +			return -1;
> >>>> +	}
> >>>
> >>
> >> Thanks for replying :-)
> >>
> >>> first, please note that there's support for perf.data directory code,
> >>> but it's not been enabled yet, so we can do any changes there without
> >>> breaking existing users
> >>>
> >>> currently the logic is prepared to have perf.data DIR_FORMAT feature
> >>> to define the layout of the directory
> >>>
> >>> it'd be great to have just single point where we get directory layout,
> >>> not checking on files names first and checking on DIR_FORMAT later
> >>
> >> Ok, but what are you suggesting?  Naming the data file "header" seems a bit
> >> counter-intuitive in this case.
> > 
> > don't know ;-)
> 
> So what about calling it "data" instead of "header"?

ok, it actualy contains data in threaded record as well,
so no problem there..

> 
> > 
> > but I'd like to have one way of finding out the directory layout
> > 
> > the code for threaded record uses DIR_FORMAT feature value
> > to ensure the directory contains the expected files, which
> > is data file with 'data.<cpu>' name for every cpu
> > 
> >>
> >>>
> >>> also the kcore will be beneficial for other layouts,
> >>> so would be great to make it somehow optional/switchable
> >>
> >> In these patches it is, because it is not related to the DIR_FORMAT.
> >>
> >>> one of the options could be to have DIR_FORMAT feature as the source
> >>> of directory layout and it'd have bitmask of files/dirs (like kcore_dir)
> >>> available in the directory
> >>
> >> Is there an advantage to making optional files/dirs part of the format?
> >> i.e. if they are there, use them otherwise don't.
> > 
> > ok, that might work, but please make that somehow explicit/visible
> > what files/directories are possible in the directory, so we could
> > easily see them and add new ones
> 
> At the moment, what can exist is what can be removed i.e. see
> rm_rf_perf_data().  Will that do?

ok, but also please some comments in data.h and perf.data doc update ;-)

jirka