[PATCH 1/2] perf tools: Drop strdup in get_filename_for_perf_kvm().

[PATCH 1/2] perf tools: Drop strdup in get_filename_for_perf_kvm().

[Dongsheng Yang]

As we need a const char * as a result of get_filename_for_perf_kvm(),
There is no need to use strdup() for the return value.

This patch drop the strdup() to save memory in get_filename_for_perf_kvm().

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

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index c6fa3cb..03bd946 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1712,15 +1712,9 @@ int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (!perf_host)
 		perf_guest = 1;
 
-	if (!file_name) {
+	if (!file_name)
 		file_name = get_filename_for_perf_kvm();
 
-		if (!file_name) {
-			pr_err("Failed to allocate memory for filename\n");
-			return -ENOMEM;
-		}
-	}
-
 	if (!strncmp(argv[0], "rec", 3))
 		return __cmd_record(file_name, argc, argv);
 	else if (!strncmp(argv[0], "rep", 3))
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 4a57609..e9cb136 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -488,11 +488,11 @@ const char *get_filename_for_perf_kvm(void)
 	const char *filename;
 
 	if (perf_host && !perf_guest)
-		filename = strdup("perf.data.host");
+		filename = "perf.data.host";
 	else if (!perf_host && perf_guest)
-		filename = strdup("perf.data.guest");
+		filename = "perf.data.guest";
 	else
-		filename = strdup("perf.data.kvm");
+		filename = "perf.data.kvm";
 
 	return filename;
 }
-- 
1.8.2.1

[PATCH 2/2] 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>
---
 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..a1d59d8 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 = strdup(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 1/2] perf tools: Drop strdup in get_filename_for_perf_kvm().

[Dongsheng Yang]

Arnaldo or David,
     Could you help to review this patchset. Thanx.

- Yang

On 12/11/2013 12:47 PM, Dongsheng Yang wrote:
> As we need a const char * as a result of get_filename_for_perf_kvm(),
> There is no need to use strdup() for the return value.
>
> This patch drop the strdup() to save memory in get_filename_for_perf_kvm().
>
> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
> ---
>   tools/perf/builtin-kvm.c | 8 +-------
>   tools/perf/util/util.c   | 6 +++---
>   2 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index c6fa3cb..03bd946 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -1712,15 +1712,9 @@ int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
>   	if (!perf_host)
>   		perf_guest = 1;
>   
> -	if (!file_name) {
> +	if (!file_name)
>   		file_name = get_filename_for_perf_kvm();
>   
> -		if (!file_name) {
> -			pr_err("Failed to allocate memory for filename\n");
> -			return -ENOMEM;
> -		}
> -	}
> -
>   	if (!strncmp(argv[0], "rec", 3))
>   		return __cmd_record(file_name, argc, argv);
>   	else if (!strncmp(argv[0], "rep", 3))
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 4a57609..e9cb136 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -488,11 +488,11 @@ const char *get_filename_for_perf_kvm(void)
>   	const char *filename;
>   
>   	if (perf_host && !perf_guest)
> -		filename = strdup("perf.data.host");
> +		filename = "perf.data.host";
>   	else if (!perf_host && perf_guest)
> -		filename = strdup("perf.data.guest");
> +		filename = "perf.data.guest";
>   	else
> -		filename = strdup("perf.data.kvm");
> +		filename = "perf.data.kvm";
>   
>   	return filename;
>   }