[PATCH 1/3] perf tools: Move code to generate filename for perf-kvm to function.

[PATCH 1/3] perf tools: Move code to generate filename for perf-kvm to function.

[Dongsheng Yang]

The code in builtin-kvm.c to generate filename for perf-kvm is useful
to other command such as builtin-diff.

This patch move the related code form builtin-kvm.c to util/util.c and
wrap them in a function named get_filename_for_perf_kvm.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/builtin-kvm.c |  7 +------
 tools/perf/util/util.c   | 14 ++++++++++++++
 tools/perf/util/util.h   |  2 ++
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index f8bf5f2..143ad99 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1711,12 +1711,7 @@ int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
 		perf_guest = 1;
 
 	if (!file_name) {
-		if (perf_host && !perf_guest)
-			file_name = strdup("perf.data.host");
-		else if (!perf_host && perf_guest)
-			file_name = strdup("perf.data.guest");
-		else
-			file_name = strdup("perf.data.kvm");
+		file_name = get_filename_for_perf_kvm();
 
 		if (!file_name) {
 			pr_err("Failed to allocate memory for filename\n");
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index bae8756..4a57609 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -482,3 +482,17 @@ int filename__read_str(const char *filename, char **buf, size_t *sizep)
 	close(fd);
 	return err;
 }
+
+const char *get_filename_for_perf_kvm(void)
+{
+	const char *filename;
+
+	if (perf_host && !perf_guest)
+		filename = strdup("perf.data.host");
+	else if (!perf_host && perf_guest)
+		filename = strdup("perf.data.guest");
+	else
+		filename = strdup("perf.data.kvm");
+
+	return filename;
+}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index adb39f2..e6d8796 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -309,4 +309,6 @@ void free_srcline(char *srcline);
 
 int filename__read_int(const char *filename, int *value);
 int filename__read_str(const char *filename, char **buf, size_t *sizep);
+
+const char *get_filename_for_perf_kvm(void);
 #endif /* GIT_COMPAT_UTIL_H */
-- 
1.8.2.1

[PATCH 2/3] perf tools: Make perf kvm diff support --guestmount.

[Dongsheng Yang]

In manpage of perf-kvm, --guestmount is supported by diff command,
but it does not work well.

This patch change the extend the checking in buildid-diff from
guestkallsyms or guestmodules to perf_guest. Then this checking can
cover the all cases perf kvm is used for.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/builtin-diff.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 3b67ea2..2a85cc9 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -1000,8 +1000,7 @@ static int data_init(int argc, const char **argv)
 			data__files_cnt = argc;
 			use_default = false;
 		}
-	} else if (symbol_conf.default_guest_vmlinux_name ||
-		   symbol_conf.default_guest_kallsyms) {
+	} else if (perf_guest) {
 		defaults[0] = "perf.data.host";
 		defaults[1] = "perf.data.guest";
 	}
-- 
1.8.2.1

[tip:perf/core] perf kvm: Make perf kvm diff support --guestmount.

[tip-bot for Dongsheng Yang]

Commit-ID:  d8d9608fdd19f85a524db0a41bc2def5c88cbdd0
Gitweb:     http://git.kernel.org/tip/d8d9608fdd19f85a524db0a41bc2def5c88cbdd0
Author:     Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
AuthorDate: Fri, 6 Dec 2013 17:25:52 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 9 Dec 2013 15:24:25 -0300

perf kvm: Make perf kvm diff support --guestmount.

In manpage of perf-kvm, --guestmount is supported by diff command, but
it does not work well.

This patch change the extend the checking in buildid-diff from
guestkallsyms or guestmodules to perf_guest. Then this checking can
cover the all cases perf kvm is used for.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Link: http://lkml.kernel.org/r/72857ed89642e0633f5e88f7e7abbc9645359e8e.1386368672.git.yangds.fnst@cn.fujitsu.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-diff.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 3b67ea2..2a85cc9 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -1000,8 +1000,7 @@ static int data_init(int argc, const char **argv)
 			data__files_cnt = argc;
 			use_default = false;
 		}
-	} else if (symbol_conf.default_guest_vmlinux_name ||
-		   symbol_conf.default_guest_kallsyms) {
+	} else if (perf_guest) {
 		defaults[0] = "perf.data.host";
 		defaults[1] = "perf.data.guest";
 	}

[PATCH 3/3] perf tools: Change the default filenames for perf kvm diff to perf.data.xxx and perf.data.xxx.old

[Dongsheng Yang]

Command perf kvm diff is used to diff perf.data.host and
perf.data.guest by default currently. But it is not a good
default behavior. We are more frequently using it to diff
the perf data files with the same type but captured in
different times, such as perf.data.guest and perf.data.guest.old.

This patch remove the hard coding of default filenames in builtin-diff.c,
and generate the suitable filename from current options in perf kvm diff
command. It makes the default behavior of perf kvm diff be more
valuable.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 tools/perf/builtin-diff.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 2a85cc9..6a32213 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -17,6 +17,7 @@
 #include "util/symbol.h"
 #include "util/util.h"
 #include "util/data.h"
+#include "linux/string.h"
 
 #include <stdlib.h>
 #include <math.h>
@@ -1001,8 +1002,28 @@ static int data_init(int argc, const char **argv)
 			use_default = false;
 		}
 	} else if (perf_guest) {
-		defaults[0] = "perf.data.host";
-		defaults[1] = "perf.data.guest";
+		char *file_name;
+		int len, ret;
+
+		file_name = (char *)get_filename_for_perf_kvm();
+		if (!file_name) {
+			pr_err("Failed to allocate memory for filename\n");
+			return -ENOMEM;
+		}
+
+		defaults[0] = strdup(file_name);
+		if (!file_name) {
+			pr_err("Failed to allocate memory for defaults[0]\n");
+			return -ENOMEM;
+		}
+
+		len = strlen(file_name);
+		ret = str_append(&file_name, &len, ".old");
+		if (ret) {
+			pr_err("Failed to allocate memory for defaults[1]\n");
+			return -ENOMEM;
+		}
+		defaults[1] = file_name;
 	}
 
 	if (sort_compute >= (unsigned int) data__files_cnt) {
-- 
1.8.2.1

[PATCH V2] perf tools: Change the default filenames for perf kvm diff to perf.data.xxx and perf.data.xxx.old

[Dongsheng Yang]

Command perf kvm diff is used to diff perf.data.host and
perf.data.guest by default currently. But it is not a good
default behavior.

Example:
	# perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.669 MB perf.data.guest (~29207 samples) ]
	# perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.669 MB perf.data.guest (~29207 samples) ]
	# perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules diff
	failed to open perf.data.host: No such file or directory
	Failed to open perf.data.host

We should keep the style of 'perf kvm diff' same with 'perf diff'.
It is used to diff files with same type but captured in different
times, perf.data and perf.data.old. So we need to make perf kvm diff
to diff perf.data.guest and perf.data.guest.old as a default behavior.

What's worse, as we have changed the behaviors of perf kvm record,
we can not get a perf.data.host easily. We have to use a --no-guest
to get a perf.data.host, it means we use perf.data.host as a default
input in 'perf kvm diff' is an odd design.

This patch remove the hard coding of default filenames in builtin-diff.c,
and generate the suitable filename from current options in perf kvm diff
command. It makes the default behavior of perf kvm diff be more valuable.

Verification:
	# perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
	  [ perf record: Woken up 1 times to write data ]
	  [ perf record: Captured and wrote 0.669 MB perf.data.guest (~29207 samples) ]
	# perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
	  [ perf record: Woken up 1 times to write data ]
	  [ perf record: Captured and wrote 0.669 MB perf.data.guest (~29207 samples) ]
	# perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules diff
	  # Event 'cycles'
	  #
	  # Baseline    Delta            Shared Object                                   Symbol
	  # ........  .......  .......................  .......................................
	  #
	    92.00%           [guest.kernel.kallsyms]  [g] rb_insert_color
	     7.51%           [guest.kernel.kallsyms]  [g] smp_apic_timer_interrupt
	     0.48%   +0.60%  [guest.kernel.kallsyms]  [g] apic_timer_interrupt
		    +16.35%  [guest.kernel.kallsyms]  [g] kvm_clock_get_cycles
		    +82.56%  [guest.kernel.kallsyms]  [g] irqtime_account_process_tick.isra.2

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
    Change since v1:
	Add more commit message.

 tools/perf/builtin-diff.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 2a85cc9..6a32213 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -17,6 +17,7 @@
 #include "util/symbol.h"
 #include "util/util.h"
 #include "util/data.h"
+#include "linux/string.h"
 
 #include <stdlib.h>
 #include <math.h>
@@ -1001,8 +1002,28 @@ static int data_init(int argc, const char **argv)
 			use_default = false;
 		}
 	} else if (perf_guest) {
-		defaults[0] = "perf.data.host";
-		defaults[1] = "perf.data.guest";
+		char *file_name;
+		int len, ret;
+
+		file_name = (char *)get_filename_for_perf_kvm();
+		if (!file_name) {
+			pr_err("Failed to allocate memory for filename\n");
+			return -ENOMEM;
+		}
+
+		defaults[0] = strdup(file_name);
+		if (!file_name) {
+			pr_err("Failed to allocate memory for defaults[0]\n");
+			return -ENOMEM;
+		}
+
+		len = strlen(file_name);
+		ret = str_append(&file_name, &len, ".old");
+		if (ret) {
+			pr_err("Failed to allocate memory for defaults[1]\n");
+			return -ENOMEM;
+		}
+		defaults[1] = file_name;
 	}
 
 	if (sort_compute >= (unsigned int) data__files_cnt) {
-- 
1.8.2.1

[PATCH V3] perf tools: Change the default filenames for perf kvm diff to perf.data.xxx and perf.data.xxx.old

[Dongsheng Yang]

Command perf kvm diff is used to diff perf.data.host and
perf.data.guest by default currently. But it is not a good
default behavior.

Example:
	# perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.669 MB perf.data.guest (~29207 samples) ]
	# perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.669 MB perf.data.guest (~29207 samples) ]
	# perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules diff
	failed to open perf.data.host: No such file or directory
	Failed to open perf.data.host

We are more frequently using it to diff the perf data files with
the same type but captured in different times, such as
perf.data.guest and perf.data.guest.old.

This patch remove the hard coding of default filenames in builtin-diff.c,
and generate the suitable filename from current options in perf kvm diff
command. It makes the default behavior of perf kvm diff be more valuable.

Verification:
	# perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
	  [ perf record: Woken up 1 times to write data ]
	  [ perf record: Captured and wrote 0.669 MB perf.data.guest (~29207 samples) ]
	# perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules record -a sleep 1
	  [ perf record: Woken up 1 times to write data ]
	  [ perf record: Captured and wrote 0.669 MB perf.data.guest (~29207 samples) ]
	# perf kvm --guestkallsyms /home/kallsyms --guestmodules /home/modules diff
	  # Event 'cycles'
	  #
	  # Baseline    Delta            Shared Object                                   Symbol
	  # ........  .......  .......................  .......................................
	  #
	    92.00%           [guest.kernel.kallsyms]  [g] rb_insert_color
	     7.51%           [guest.kernel.kallsyms]  [g] smp_apic_timer_interrupt
	     0.48%   +0.60%  [guest.kernel.kallsyms]  [g] apic_timer_interrupt
		    +16.35%  [guest.kernel.kallsyms]  [g] kvm_clock_get_cycles
		    +82.56%  [guest.kernel.kallsyms]  [g] irqtime_account_process_tick.isra.2

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
Changelog:
	changes for v3:
		*fix a copy-past bug.
	changes for v2:
		*Add more commit message.

 tools/perf/builtin-diff.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 2a85cc9..8d1b666 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -17,6 +17,7 @@
 #include "util/symbol.h"
 #include "util/util.h"
 #include "util/data.h"
+#include "linux/string.h"
 
 #include <stdlib.h>
 #include <math.h>
@@ -1001,8 +1002,28 @@ static int data_init(int argc, const char **argv)
 			use_default = false;
 		}
 	} else if (perf_guest) {
-		defaults[0] = "perf.data.host";
-		defaults[1] = "perf.data.guest";
+		char *file_name;
+		int len, ret;
+
+		file_name = (char *)get_filename_for_perf_kvm();
+		if (!file_name) {
+			pr_err("Failed to allocate memory for filename\n");
+			return -ENOMEM;
+		}
+
+		defaults[0] = strdup(file_name);
+		if (!defaults[0]) {
+			pr_err("Failed to allocate memory for defaults[0]\n");
+			return -ENOMEM;
+		}
+
+		len = strlen(file_name);
+		ret = str_append(&file_name, &len, ".old");
+		if (ret) {
+			pr_err("Failed to allocate memory for defaults[1]\n");
+			return -ENOMEM;
+		}
+		defaults[1] = file_name;
 	}
 
 	if (sort_compute >= (unsigned int) data__files_cnt) {
-- 
1.8.2.1

Re: [PATCH V3] perf tools: Change the default filenames for perf kvm diff to perf.data.xxx and perf.data.xxx.old

[David Ahern]

On 12/11/13, 9:30 AM, Dongsheng Yang wrote:
> @@ -1001,8 +1002,28 @@ static int data_init(int argc, const char **argv)
>   			use_default = false;
>   		}
>   	} else if (perf_guest) {
> -		defaults[0] = "perf.data.host";
> -		defaults[1] = "perf.data.guest";
> +		char *file_name;
> +		int len, ret;
> +
> +		file_name = (char *)get_filename_for_perf_kvm();
> +		if (!file_name) {
> +			pr_err("Failed to allocate memory for filename\n");
> +			return -ENOMEM;
> +		}
> +

The need for a typecast should tell you something is wrong. Why is 
get_filename_for_perf_kvm returning a const char * when it is allocated 
memory?

David

Re: [PATCH V3] perf tools: Change the default filenames for perf kvm diff to perf.data.xxx and perf.data.xxx.old

[Dongsheng Yang]

On 12/10/2013 10:38 PM, David Ahern wrote:
> On 12/11/13, 9:30 AM, Dongsheng Yang wrote:
>> @@ -1001,8 +1002,28 @@ static int data_init(int argc, const char **argv)
>>               use_default = false;
>>           }
>>       } else if (perf_guest) {
>> -        defaults[0] = "perf.data.host";
>> -        defaults[1] = "perf.data.guest";
>> +        char *file_name;
>> +        int len, ret;
>> +
>> +        file_name = (char *)get_filename_for_perf_kvm();
>> +        if (!file_name) {
>> +            pr_err("Failed to allocate memory for filename\n");
>> +            return -ENOMEM;
>> +        }
>> +
>
> The need for a typecast should tell you something is wrong. Why is 
> get_filename_for_perf_kvm returning a const char * when it is 
> allocated memory?
>

Yes, there is something is wrong I think. It returning const char* 
because I assume the file_name will never be changed. But now, this 
assumption seems outdated.

I will add a new patch to change the return value to char *, as the 
other two patches is already applied.
http://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=e1a2b174dbbe08dce12bde9f05f64dbbae652bed

Thanx

Yang
> David
>

[tip:perf/core] perf kvm: Move code to generate filename for perf-kvm to function.

[tip-bot for Dongsheng Yang]

Commit-ID:  e1a2b174dbbe08dce12bde9f05f64dbbae652bed
Gitweb:     http://git.kernel.org/tip/e1a2b174dbbe08dce12bde9f05f64dbbae652bed
Author:     Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
AuthorDate: Fri, 6 Dec 2013 17:25:51 -0500
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 9 Dec 2013 15:23:56 -0300

perf kvm: Move code to generate filename for perf-kvm to function.

The code in builtin-kvm.c to generate filename for perf-kvm is useful to
other command such as builtin-diff.

This patch move the related code form builtin-kvm.c to util/util.c and
wrap them in a function named get_filename_for_perf_kvm.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Link: http://lkml.kernel.org/r/5e09a5c47e8a495e888cbdc65a6fafb2c950f529.1386368672.git.yangds.fnst@cn.fujitsu.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-kvm.c |  7 +------
 tools/perf/util/util.c   | 14 ++++++++++++++
 tools/perf/util/util.h   |  2 ++
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index c2e5d56..c6fa3cb 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1713,12 +1713,7 @@ int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
 		perf_guest = 1;
 
 	if (!file_name) {
-		if (perf_host && !perf_guest)
-			file_name = strdup("perf.data.host");
-		else if (!perf_host && perf_guest)
-			file_name = strdup("perf.data.guest");
-		else
-			file_name = strdup("perf.data.kvm");
+		file_name = get_filename_for_perf_kvm();
 
 		if (!file_name) {
 			pr_err("Failed to allocate memory for filename\n");
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index bae8756..4a57609 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -482,3 +482,17 @@ int filename__read_str(const char *filename, char **buf, size_t *sizep)
 	close(fd);
 	return err;
 }
+
+const char *get_filename_for_perf_kvm(void)
+{
+	const char *filename;
+
+	if (perf_host && !perf_guest)
+		filename = strdup("perf.data.host");
+	else if (!perf_host && perf_guest)
+		filename = strdup("perf.data.guest");
+	else
+		filename = strdup("perf.data.kvm");
+
+	return filename;
+}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 659abf3..0171213 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -321,4 +321,6 @@ void free_srcline(char *srcline);
 
 int filename__read_int(const char *filename, int *value);
 int filename__read_str(const char *filename, char **buf, size_t *sizep);
+
+const char *get_filename_for_perf_kvm(void);
 #endif /* GIT_COMPAT_UTIL_H */