[PATCH 1/3] perf ftrace: Remove needless code setting default tracer

[PATCH 1/3] perf ftrace: Remove needless code setting default tracer

[Taeung Song]

Lately commit a349764 made 'function_graph' be the default tracer.
So ftrace.tracer variable can't be NULL but the other code setting default
tracer remained. For this reason, remove it as below.
And use existing DEFAULT_TRACER instead of "function_graph".

Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-ftrace.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index d05658d..414444d 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -202,7 +202,7 @@ int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	int ret;
 	struct perf_ftrace ftrace = {
-		.tracer = "function_graph",
+		.tracer = DEFAULT_TRACER,
 		.target = { .uid = UINT_MAX, },
 	};
 	const char * const ftrace_usage[] = {
@@ -231,9 +231,6 @@ int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (ret < 0)
 		goto out_delete_evlist;
 
-	if (ftrace.tracer == NULL)
-		ftrace.tracer = DEFAULT_TRACER;
-
 	ret = __cmd_ftrace(&ftrace, argc, argv);
 
 out_delete_evlist:
-- 
2.7.4

[PATCH 2/3] perf ftrace: Add ftrace.tracer config option

[Taeung Song]

Currently perf ftrace command will select 'function_graph' or 'function'.
So add ftrace.tracer config option to select tracer

    # cat ~/.perfconfig
    [ftrace]
        tracer = function

    # perf ftrace usleep 123456 | head -10
      <...>-14450 [002] d... 10089.284231: finish_task_switch <-__schedule
      <...>-14450 [002] .... 10089.284232: finish_wait <-pipe_wait
      <...>-14450 [002] .... 10089.284232: mutex_lock <-pipe_wait
      <...>-14450 [002] .... 10089.284232: _cond_resched <-mutex_lock
      <...>-14450 [002] .... 10089.284233: generic_pipe_buf_confirm <-pipe_read

Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-ftrace.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 414444d..8df5416 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -17,6 +17,7 @@
 #include "evlist.h"
 #include "target.h"
 #include "thread_map.h"
+#include "util/config.h"
 
 
 #define DEFAULT_TRACER  "function_graph"
@@ -198,6 +199,23 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace, int argc, const char **argv)
 	return done ? 0 : -1;
 }
 
+static int perf_ftrace_config(const char *var, const char *value, void *cb)
+{
+	struct perf_ftrace *ftrace = cb;
+
+	if (!strcmp(var, "ftrace.tracer")) {
+		if (!strcmp(value, "function_graph"))
+			ftrace->tracer = DEFAULT_TRACER;
+		else if (!strcmp(value, "function"))
+			ftrace->tracer = "function";
+		else
+			return -1;
+		return 0;
+	}
+
+	return 0;
+}
+
 int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	int ret;
@@ -218,6 +236,8 @@ int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
 	OPT_END()
 	};
 
+	perf_config(perf_ftrace_config, &ftrace);
+
 	argc = parse_options(argc, argv, ftrace_options, ftrace_usage,
 			    PARSE_OPT_STOP_AT_NON_OPTION);
 	if (!argc)
-- 
2.7.4

[PATCH 3/3] perf ftrace: Handle NULL at write_tracing_file()

[Taeung Song]

If a value for tracing file is NULL,
segment fault error can occur using strlen().
Of course, currently the function don't be given NULL value.
But write_tracing_file() can be generally used.
So add the if statement.

Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
 tools/perf/builtin-ftrace.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 8df5416..0754dee 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -54,8 +54,12 @@ static int write_tracing_file(const char *name, const char *val)
 {
 	char *file;
 	int fd, ret = -1;
-	ssize_t size = strlen(val);
+	ssize_t size;
 
+	if (val == NULL)
+		return -1;
+
+	size = strlen(val);
 	file = get_tracing_file(name);
 	if (!file) {
 		pr_debug("cannot get tracing file: %s\n", name);
-- 
2.7.4

Re: [PATCH 1/3] perf ftrace: Remove needless code setting default tracer

[Arnaldo Carvalho de Melo]

Em Thu, Jan 26, 2017 at 06:35:37PM +0900, Taeung Song escreveu:
> Lately commit a349764 made 'function_graph' be the default tracer.
> So ftrace.tracer variable can't be NULL but the other code setting default
> tracer remained. For this reason, remove it as below.
> And use existing DEFAULT_TRACER instead of "function_graph".

Rewrote a bit the commit log, applied,

- Arnaldo
 
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/builtin-ftrace.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index d05658d..414444d 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -202,7 +202,7 @@ int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
>  {
>  	int ret;
>  	struct perf_ftrace ftrace = {
> -		.tracer = "function_graph",
> +		.tracer = DEFAULT_TRACER,
>  		.target = { .uid = UINT_MAX, },
>  	};
>  	const char * const ftrace_usage[] = {
> @@ -231,9 +231,6 @@ int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
>  	if (ret < 0)
>  		goto out_delete_evlist;
>  
> -	if (ftrace.tracer == NULL)
> -		ftrace.tracer = DEFAULT_TRACER;
> -
>  	ret = __cmd_ftrace(&ftrace, argc, argv);
>  
>  out_delete_evlist:
> -- 
> 2.7.4

Re: [PATCH 2/3] perf ftrace: Add ftrace.tracer config option

[Arnaldo Carvalho de Melo]

Em Thu, Jan 26, 2017 at 06:35:38PM +0900, Taeung Song escreveu:
> Currently perf ftrace command will select 'function_graph' or 'function'.
> So add ftrace.tracer config option to select tracer
> 
>     # cat ~/.perfconfig
>     [ftrace]
>         tracer = function
> 
>     # perf ftrace usleep 123456 | head -10
>       <...>-14450 [002] d... 10089.284231: finish_task_switch <-__schedule
>       <...>-14450 [002] .... 10089.284232: finish_wait <-pipe_wait
>       <...>-14450 [002] .... 10089.284232: mutex_lock <-pipe_wait
>       <...>-14450 [002] .... 10089.284232: _cond_resched <-mutex_lock
>       <...>-14450 [002] .... 10089.284233: generic_pipe_buf_confirm <-pipe_read
> 
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
>  tools/perf/builtin-ftrace.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 414444d..8df5416 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -17,6 +17,7 @@
>  #include "evlist.h"
>  #include "target.h"
>  #include "thread_map.h"
> +#include "util/config.h"
>  
>  
>  #define DEFAULT_TRACER  "function_graph"
> @@ -198,6 +199,23 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace, int argc, const char **argv)
>  	return done ? 0 : -1;
>  }
>  
> +static int perf_ftrace_config(const char *var, const char *value, void *cb)
> +{
> +	struct perf_ftrace *ftrace = cb;
> +
> +	if (!strcmp(var, "ftrace.tracer")) {
> +		if (!strcmp(value, "function_graph"))
> +			ftrace->tracer = DEFAULT_TRACER;
> +		else if (!strcmp(value, "function"))
> +			ftrace->tracer = "function";
> +		else
> +			return -1;

Please warn the user for invalid values and either just say that it is
ignoring that setting or return -1 to make perf_config() return that,
then make cmd_ftrace() check the return of perf_config() and bail out if
you're not ignoring the config value.

I think its better to do a: "pr_err()" in that "return -1" branch and
make 'perf ftrace' fail, so that the user can fix its config before
proceeding.

- Arnaldo

> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +
>  int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
>  {
>  	int ret;
> @@ -218,6 +236,8 @@ int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
>  	OPT_END()
>  	};
>  
> +	perf_config(perf_ftrace_config, &ftrace);
> +
>  	argc = parse_options(argc, argv, ftrace_options, ftrace_usage,
>  			    PARSE_OPT_STOP_AT_NON_OPTION);
>  	if (!argc)
> -- 
> 2.7.4

Re: [PATCH 2/3] perf ftrace: Add ftrace.tracer config option

[Taeung Song]

Hi, Arnaldo :)

On 01/27/2017 03:58 AM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 26, 2017 at 06:35:38PM +0900, Taeung Song escreveu:
>> Currently perf ftrace command will select 'function_graph' or 'function'.
>> So add ftrace.tracer config option to select tracer
>>
>>     # cat ~/.perfconfig
>>     [ftrace]
>>         tracer = function
>>
>>     # perf ftrace usleep 123456 | head -10
>>       <...>-14450 [002] d... 10089.284231: finish_task_switch <-__schedule
>>       <...>-14450 [002] .... 10089.284232: finish_wait <-pipe_wait
>>       <...>-14450 [002] .... 10089.284232: mutex_lock <-pipe_wait
>>       <...>-14450 [002] .... 10089.284232: _cond_resched <-mutex_lock
>>       <...>-14450 [002] .... 10089.284233: generic_pipe_buf_confirm <-pipe_read
>>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>  tools/perf/builtin-ftrace.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
>> index 414444d..8df5416 100644
>> --- a/tools/perf/builtin-ftrace.c
>> +++ b/tools/perf/builtin-ftrace.c
>> @@ -17,6 +17,7 @@
>>  #include "evlist.h"
>>  #include "target.h"
>>  #include "thread_map.h"
>> +#include "util/config.h"
>>
>>
>>  #define DEFAULT_TRACER  "function_graph"
>> @@ -198,6 +199,23 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace, int argc, const char **argv)
>>  	return done ? 0 : -1;
>>  }
>>
>> +static int perf_ftrace_config(const char *var, const char *value, void *cb)
>> +{
>> +	struct perf_ftrace *ftrace = cb;
>> +
>> +	if (!strcmp(var, "ftrace.tracer")) {
>> +		if (!strcmp(value, "function_graph"))
>> +			ftrace->tracer = DEFAULT_TRACER;
>> +		else if (!strcmp(value, "function"))
>> +			ftrace->tracer = "function";
>> +		else
>> +			return -1;
>
> Please warn the user for invalid values and either just say that it is
> ignoring that setting or return -1 to make perf_config() return that,
> then make cmd_ftrace() check the return of perf_config() and bail out if
> you're not ignoring the config value.
>
> I think its better to do a: "pr_err()" in that "return -1" branch and
> make 'perf ftrace' fail, so that the user can fix its config before
> proceeding.
>

I understood it!
I'll modify this patch and resend it. :)

Thanks,
Taeung

Re: [PATCH 2/3] perf ftrace: Add ftrace.tracer config option

[Taeung Song]

On 01/27/2017 03:58 AM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jan 26, 2017 at 06:35:38PM +0900, Taeung Song escreveu:
>> Currently perf ftrace command will select 'function_graph' or 'function'.
>> So add ftrace.tracer config option to select tracer
>>
>>     # cat ~/.perfconfig
>>     [ftrace]
>>         tracer = function
>>
>>     # perf ftrace usleep 123456 | head -10
>>       <...>-14450 [002] d... 10089.284231: finish_task_switch <-__schedule
>>       <...>-14450 [002] .... 10089.284232: finish_wait <-pipe_wait
>>       <...>-14450 [002] .... 10089.284232: mutex_lock <-pipe_wait
>>       <...>-14450 [002] .... 10089.284232: _cond_resched <-mutex_lock
>>       <...>-14450 [002] .... 10089.284233: generic_pipe_buf_confirm <-pipe_read
>>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>>  tools/perf/builtin-ftrace.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
>> index 414444d..8df5416 100644
>> --- a/tools/perf/builtin-ftrace.c
>> +++ b/tools/perf/builtin-ftrace.c
>> @@ -17,6 +17,7 @@
>>  #include "evlist.h"
>>  #include "target.h"
>>  #include "thread_map.h"
>> +#include "util/config.h"
>>
>>
>>  #define DEFAULT_TRACER  "function_graph"
>> @@ -198,6 +199,23 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace, int argc, const char **argv)
>>  	return done ? 0 : -1;
>>  }
>>
>> +static int perf_ftrace_config(const char *var, const char *value, void *cb)
>> +{
>> +	struct perf_ftrace *ftrace = cb;
>> +
>> +	if (!strcmp(var, "ftrace.tracer")) {
>> +		if (!strcmp(value, "function_graph"))
>> +			ftrace->tracer = DEFAULT_TRACER;
>> +		else if (!strcmp(value, "function"))
>> +			ftrace->tracer = "function";
>> +		else
>> +			return -1;
>
> Please warn the user for invalid values and either just say that it is
> ignoring that setting or return -1 to make perf_config() return that,
> then make cmd_ftrace() check the return of perf_config() and bail out if
> you're not ignoring the config value.
>
> I think its better to do a: "pr_err()" in that "return -1" branch and
> make 'perf ftrace' fail, so that the user can fix its config before
> proceeding.
>


Arnaldo,

I modified this patch as below.

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 8df5416..00e228f 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -208,8 +208,11 @@ static int perf_ftrace_config(const char *var, 
const char *value, void *cb)
                          ftrace->tracer = DEFAULT_TRACER;
                  else if (!strcmp(value, "function"))
                          ftrace->tracer = "function";
-                else
+                else {
+                        pr_err("Please select function_graph(default)"
+                               "or function to use tracer.\n");
                          return -1;
+                }
                  return 0;
          }

@@ -236,7 +239,9 @@ int cmd_ftrace(int argc, const char **argv, const 
char *prefix __maybe_unused)
          OPT_END()
          };

-        perf_config(perf_ftrace_config, &ftrace);
+        ret = perf_config(perf_ftrace_config, &ftrace);
+        if (ret < 0)
+                return -1;

          argc = parse_options(argc, argv, ftrace_options, ftrace_usage,
                              PARSE_OPT_STOP_AT_NON_OPTION);


But if setting wrong config value,
existing error message is printed in perf_config().

So output can be printed as follows

1) Two error messages

     # perf ftrace usleep 123456
     Please select function_graph(default)or function to use tracer.
     Error: wrong config key-value pair ftrace.tracer=functionds

2) Or only one error message in perf_config()
(if we don't "pr_err()" in that "return -1" branch)

     # perf ftrace usleep 123456
     Error: wrong config key-value pair ftrace.tracer=functionds


Which do you like better, 1) or 2) ?

Thanks,
Taeung

[tip:perf/core] perf ftrace: Remove needless code setting default tracer

[tip-bot for Taeung Song]

Commit-ID:  bf062bd20e6c4988a9c593824ea6bb58730b6289
Gitweb:     http://git.kernel.org/tip/bf062bd20e6c4988a9c593824ea6bb58730b6289
Author:     Taeung Song <treeze.taeung@gmail.com>
AuthorDate: Thu, 26 Jan 2017 18:35:37 +0900
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 26 Jan 2017 15:53:27 -0300

perf ftrace: Remove needless code setting default tracer

As a result of commit a3497642c261 ("perf ftrace: Make 'function_graph'
be the default tracer") the ftrace.tracer variable can't be NULL but the
other code setting default tracer remained.

Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Wang Nan <wangnan0@huawei.com>
Link: http://lkml.kernel.org/r/1485423339-22780-1-git-send-email-treeze.taeung@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-ftrace.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index d05658d..414444d 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -202,7 +202,7 @@ int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
 {
 	int ret;
 	struct perf_ftrace ftrace = {
-		.tracer = "function_graph",
+		.tracer = DEFAULT_TRACER,
 		.target = { .uid = UINT_MAX, },
 	};
 	const char * const ftrace_usage[] = {
@@ -231,9 +231,6 @@ int cmd_ftrace(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (ret < 0)
 		goto out_delete_evlist;
 
-	if (ftrace.tracer == NULL)
-		ftrace.tracer = DEFAULT_TRACER;
-
 	ret = __cmd_ftrace(&ftrace, argc, argv);
 
 out_delete_evlist: