[PATCH -next 0/5] perf: Minor fixes and cleanup

[PATCH -next 0/5] perf: Minor fixes and cleanup

[Chen Zhongjin]

This series fixes and cleans up some minor issues for perf-event and
perf-trace. Mainly about cleaning dead code and macros.

Chen Zhongjin (5):
  perf: Fix show_arg_names not working for tp arg names
  perf: Fix incorrectly parsed flags in filter
  perf: Remove duplicate errbuf
  perf: Remove unused macros __PERF_EVENT_FIELD
  perf: Remove unused macro K

 tools/perf/builtin-trace.c     | 20 +++++---------------
 tools/perf/util/parse-events.c |  8 --------
 tools/perf/util/string.c       |  1 -
 3 files changed, 5 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH -next 1/5] perf: Fix show_arg_names not working for tp arg names

[Chen Zhongjin]

trace__fprintf_tp_fields() will always print arg names because when
implemented it is forced to print arg_names with:
(1 || trace->show_arg_names)

So the printing looks like:

> cat ~/.perfconfig
    [trace]
        show_arg_names = no

> perf trace -e syscalls:*mmap sleep 1
    0.000 sleep/1119 syscalls:sys_enter_mmap(NULL, 8192, READ|WRITE, PRIVATE|ANONYMOUS)
    0.179 sleep/1119 syscalls:sys_exit_mmap(__syscall_nr: 9, ret: 140535426170880)
    ...

Although the comment said that perhaps we need a show_tp_arg_names.
I don't think it's necessary to control them separately because
it's not so clean that part of the log shows arg names but other not.
Also when we are tracing functions it's rare to especially distinguish
syscalls and tp trace.

Only use one option to control arg names printing is more resonable
and simple. So remove the force condition and commit.

After fix:

> perf trace -e syscalls:*mmap sleep 1
    0.000 sleep/1121 syscalls:sys_enter_mmap(NULL, 8192, READ|WRITE, PRIVATE|ANONYMOUS)
    0.163 sleep/1121 syscalls:sys_exit_mmap(9, 140454467661824)
    ...

Fixes: f11b2803bb88 ("perf trace: Allow choosing how to augment the tracepoint arguments")
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 tools/perf/builtin-trace.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 0bd9d01c0df9..c7bb7a8222a6 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -2762,11 +2762,7 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel,
 
 		printed += scnprintf(bf + printed, size - printed, "%s", printed ? ", " : "");
 
-		/*
-		 * XXX Perhaps we should have a show_tp_arg_names,
-		 * leaving show_arg_names just for syscalls?
-		 */
-		if (1 || trace->show_arg_names)
+		if (trace->show_arg_names)
 			printed += scnprintf(bf + printed, size - printed, "%s: ", field->name);
 
 		printed += syscall_arg_fmt__scnprintf_val(arg, bf + printed, size - printed, &syscall_arg, val);
-- 
2.17.1

Re: [PATCH -next 1/5] perf: Fix show_arg_names not working for tp arg names

[Arnaldo Carvalho de Melo]

Em Mon, Sep 26, 2022 at 11:14:36AM +0800, Chen Zhongjin escreveu:
> trace__fprintf_tp_fields() will always print arg names because when
> implemented it is forced to print arg_names with:
> (1 || trace->show_arg_names)
> 
> So the printing looks like:
> 
> > cat ~/.perfconfig
>     [trace]
>         show_arg_names = no
> 
> > perf trace -e syscalls:*mmap sleep 1
>     0.000 sleep/1119 syscalls:sys_enter_mmap(NULL, 8192, READ|WRITE, PRIVATE|ANONYMOUS)
>     0.179 sleep/1119 syscalls:sys_exit_mmap(__syscall_nr: 9, ret: 140535426170880)
>     ...
> 
> Although the comment said that perhaps we need a show_tp_arg_names.
> I don't think it's necessary to control them separately because
> it's not so clean that part of the log shows arg names but other not.
> Also when we are tracing functions it's rare to especially distinguish
> syscalls and tp trace.
> 
> Only use one option to control arg names printing is more resonable
> and simple. So remove the force condition and commit.
> 
> After fix:
 
> > perf trace -e syscalls:*mmap sleep 1
>     0.000 sleep/1121 syscalls:sys_enter_mmap(NULL, 8192, READ|WRITE, PRIVATE|ANONYMOUS)
>     0.163 sleep/1121 syscalls:sys_exit_mmap(9, 140454467661824)
>     ...

Thanks, applied.

- Arnaldo

 
> Fixes: f11b2803bb88 ("perf trace: Allow choosing how to augment the tracepoint arguments")
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>  tools/perf/builtin-trace.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 0bd9d01c0df9..c7bb7a8222a6 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -2762,11 +2762,7 @@ static size_t trace__fprintf_tp_fields(struct trace *trace, struct evsel *evsel,
>  
>  		printed += scnprintf(bf + printed, size - printed, "%s", printed ? ", " : "");
>  
> -		/*
> -		 * XXX Perhaps we should have a show_tp_arg_names,
> -		 * leaving show_arg_names just for syscalls?
> -		 */
> -		if (1 || trace->show_arg_names)
> +		if (trace->show_arg_names)
>  			printed += scnprintf(bf + printed, size - printed, "%s: ", field->name);
>  
>  		printed += syscall_arg_fmt__scnprintf_val(arg, bf + printed, size - printed, &syscall_arg, val);
> -- 
> 2.17.1

-- 

- Arnaldo

[PATCH -next 2/5] perf: Fix incorrectly parsed flags in filter

[Chen Zhongjin]

When parsing flags in filter, the strtoul function uses wrong parsing
condition (tok[1] = 'x'), which can make the flags be corrupted and
treat all numbers start with 0 as hex.

In fact strtoul() will auto test hex format when base == 0 (See
_parse_integer_fixup_radix). So there is no need to test this again.

Remove the unnessesary is_hexa test.

Fixes: 154c978d484c ("libbeauty: Introduce strarray__strtoul_flags()")
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 tools/perf/builtin-trace.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index c7bb7a8222a6..7ecd76428440 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -615,11 +615,8 @@ bool strarray__strtoul_flags(struct strarray *sa, char *bf, size_t size, u64 *re
 		if (isalpha(*tok) || *tok == '_') {
 			if (!strarray__strtoul(sa, tok, toklen, &val))
 				return false;
-		} else {
-			bool is_hexa = tok[0] == 0 && (tok[1] = 'x' || tok[1] == 'X');
-
-			val = strtoul(tok, NULL, is_hexa ? 16 : 0);
-		}
+		} else
+			val = strtoul(tok, NULL, 0);
 
 		*ret |= (1 << (val - 1));
 
-- 
2.17.1

Re: [PATCH -next 2/5] perf: Fix incorrectly parsed flags in filter

[Arnaldo Carvalho de Melo]

Em Mon, Sep 26, 2022 at 11:14:37AM +0800, Chen Zhongjin escreveu:
> When parsing flags in filter, the strtoul function uses wrong parsing
> condition (tok[1] = 'x'), which can make the flags be corrupted and
> treat all numbers start with 0 as hex.
> 
> In fact strtoul() will auto test hex format when base == 0 (See
> _parse_integer_fixup_radix). So there is no need to test this again.
> 
> Remove the unnessesary is_hexa test.

Thanks, applied.

- Arnaldo

 
> Fixes: 154c978d484c ("libbeauty: Introduce strarray__strtoul_flags()")
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>  tools/perf/builtin-trace.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index c7bb7a8222a6..7ecd76428440 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -615,11 +615,8 @@ bool strarray__strtoul_flags(struct strarray *sa, char *bf, size_t size, u64 *re
>  		if (isalpha(*tok) || *tok == '_') {
>  			if (!strarray__strtoul(sa, tok, toklen, &val))
>  				return false;
> -		} else {
> -			bool is_hexa = tok[0] == 0 && (tok[1] = 'x' || tok[1] == 'X');
> -
> -			val = strtoul(tok, NULL, is_hexa ? 16 : 0);
> -		}
> +		} else
> +			val = strtoul(tok, NULL, 0);
>  
>  		*ret |= (1 << (val - 1));
>  
> -- 
> 2.17.1

-- 

- Arnaldo

[PATCH -next 3/5] perf: Remove duplicate errbuf

[Chen Zhongjin]

char errbuf[BUFSIZ] is defined twice in trace__run.

However out_error_open is not cross to other out_error path, they can
share one errbuf together.

Define the errbuf[BUFSIZ] at the beginning of function, and remove the
redefinations of them for code cleaning.

Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 tools/perf/builtin-trace.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 7ecd76428440..5660c0ee3507 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -3937,6 +3937,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
 	unsigned long before;
 	const bool forks = argc > 0;
 	bool draining = false;
+	char errbuf[BUFSIZ];
 
 	trace->live = true;
 
@@ -4027,8 +4028,6 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
 
 	err = bpf__apply_obj_config();
 	if (err) {
-		char errbuf[BUFSIZ];
-
 		bpf__strerror_apply_obj_config(err, errbuf, sizeof(errbuf));
 		pr_err("ERROR: Apply config to BPF failed: %s\n",
 			 errbuf);
@@ -4185,8 +4184,6 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
 	trace->evlist = NULL;
 	trace->live = false;
 	return err;
-{
-	char errbuf[BUFSIZ];
 
 out_error_sched_stat_runtime:
 	tracing_path__strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
@@ -4213,7 +4210,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
 		evsel->filter, evsel__name(evsel), errno,
 		str_error_r(errno, errbuf, sizeof(errbuf)));
 	goto out_delete_evlist;
-}
+
 out_error_mem:
 	fprintf(trace->output, "Not enough memory to run!\n");
 	goto out_delete_evlist;
-- 
2.17.1

Re: [PATCH -next 3/5] perf: Remove duplicate errbuf

[Arnaldo Carvalho de Melo]

Em Mon, Sep 26, 2022 at 11:14:38AM +0800, Chen Zhongjin escreveu:
> char errbuf[BUFSIZ] is defined twice in trace__run.
> 
> However out_error_open is not cross to other out_error path, they can
> share one errbuf together.
> 
> Define the errbuf[BUFSIZ] at the beginning of function, and remove the
> redefinations of them for code cleaning.

Have you looked at the end result in the generated code? Just out of
curiosity.

- Arnaldo
 
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>  tools/perf/builtin-trace.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 7ecd76428440..5660c0ee3507 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -3937,6 +3937,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>  	unsigned long before;
>  	const bool forks = argc > 0;
>  	bool draining = false;
> +	char errbuf[BUFSIZ];
>  
>  	trace->live = true;
>  
> @@ -4027,8 +4028,6 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>  
>  	err = bpf__apply_obj_config();
>  	if (err) {
> -		char errbuf[BUFSIZ];
> -
>  		bpf__strerror_apply_obj_config(err, errbuf, sizeof(errbuf));
>  		pr_err("ERROR: Apply config to BPF failed: %s\n",
>  			 errbuf);
> @@ -4185,8 +4184,6 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>  	trace->evlist = NULL;
>  	trace->live = false;
>  	return err;
> -{
> -	char errbuf[BUFSIZ];
>  
>  out_error_sched_stat_runtime:
>  	tracing_path__strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
> @@ -4213,7 +4210,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>  		evsel->filter, evsel__name(evsel), errno,
>  		str_error_r(errno, errbuf, sizeof(errbuf)));
>  	goto out_delete_evlist;
> -}
> +
>  out_error_mem:
>  	fprintf(trace->output, "Not enough memory to run!\n");
>  	goto out_delete_evlist;
> -- 
> 2.17.1

-- 

- Arnaldo

Re: [PATCH -next 3/5] perf: Remove duplicate errbuf

[Chen Zhongjin]

Hi,

On 2022/9/27 3:54, Arnaldo Carvalho de Melo wrote:
> Em Mon, Sep 26, 2022 at 11:14:38AM +0800, Chen Zhongjin escreveu:
>> char errbuf[BUFSIZ] is defined twice in trace__run.
>>
>> However out_error_open is not cross to other out_error path, they can
>> share one errbuf together.
>>
>> Define the errbuf[BUFSIZ] at the beginning of function, and remove the
>> redefinations of them for code cleaning.
> Have you looked at the end result in the generated code? Just out of
> curiosity.

Good question. Because it is an auto reported warning of clang I didn't 
try it.

Just tried and found no different for 'objdump -d perf' on two version.


Best,

Chen

>
> - Arnaldo
>   
>> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
>> ---
>>   tools/perf/builtin-trace.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
>> index 7ecd76428440..5660c0ee3507 100644
>> --- a/tools/perf/builtin-trace.c
>> +++ b/tools/perf/builtin-trace.c
>> @@ -3937,6 +3937,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>>   	unsigned long before;
>>   	const bool forks = argc > 0;
>>   	bool draining = false;
>> +	char errbuf[BUFSIZ];
>>   
>>   	trace->live = true;
>>   
>> @@ -4027,8 +4028,6 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>>   
>>   	err = bpf__apply_obj_config();
>>   	if (err) {
>> -		char errbuf[BUFSIZ];
>> -
>>   		bpf__strerror_apply_obj_config(err, errbuf, sizeof(errbuf));
>>   		pr_err("ERROR: Apply config to BPF failed: %s\n",
>>   			 errbuf);
>> @@ -4185,8 +4184,6 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>>   	trace->evlist = NULL;
>>   	trace->live = false;
>>   	return err;
>> -{
>> -	char errbuf[BUFSIZ];
>>   
>>   out_error_sched_stat_runtime:
>>   	tracing_path__strerror_open_tp(errno, errbuf, sizeof(errbuf), "sched", "sched_stat_runtime");
>> @@ -4213,7 +4210,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)
>>   		evsel->filter, evsel__name(evsel), errno,
>>   		str_error_r(errno, errbuf, sizeof(errbuf)));
>>   	goto out_delete_evlist;
>> -}
>> +
>>   out_error_mem:
>>   	fprintf(trace->output, "Not enough memory to run!\n");
>>   	goto out_delete_evlist;
>> -- 
>> 2.17.1

[PATCH -next 4/5] perf: Remove unused macros __PERF_EVENT_FIELD

[Chen Zhongjin]

Unused macros reported by [-Wunused-macros].

This macros were introduced as __PERF_COUNTER_FIELD and used for
reading the bit in config.

'cdd6c482c9ff ("perf: Do the big rename: Performance Counters -> Performance Events")'
Changes it to __PERF_EVENT_FIELD but at this commit there is already
nowhere else using these macros, also no macros called
PERF_EVENT_##name##_MASK/SHIFT.

Now we are not reading type or id from config. These macros are
useless and incomplete.

So removing them for code cleaning.

Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 tools/perf/util/parse-events.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index f05e15acd33f..3ed914882b96 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -149,14 +149,6 @@ struct event_symbol event_symbols_sw[PERF_COUNT_SW_MAX] = {
 	},
 };
 
-#define __PERF_EVENT_FIELD(config, name) \
-	((config & PERF_EVENT_##name##_MASK) >> PERF_EVENT_##name##_SHIFT)
-
-#define PERF_EVENT_RAW(config)		__PERF_EVENT_FIELD(config, RAW)
-#define PERF_EVENT_CONFIG(config)	__PERF_EVENT_FIELD(config, CONFIG)
-#define PERF_EVENT_TYPE(config)		__PERF_EVENT_FIELD(config, TYPE)
-#define PERF_EVENT_ID(config)		__PERF_EVENT_FIELD(config, EVENT)
-
 const char *event_type(int type)
 {
 	switch (type) {
-- 
2.17.1

Re: [PATCH -next 4/5] perf: Remove unused macros __PERF_EVENT_FIELD

[Arnaldo Carvalho de Melo]

Em Mon, Sep 26, 2022 at 11:14:39AM +0800, Chen Zhongjin escreveu:
> Unused macros reported by [-Wunused-macros].
> 
> This macros were introduced as __PERF_COUNTER_FIELD and used for
> reading the bit in config.
> 
> 'cdd6c482c9ff ("perf: Do the big rename: Performance Counters -> Performance Events")'
> Changes it to __PERF_EVENT_FIELD but at this commit there is already
> nowhere else using these macros, also no macros called
> PERF_EVENT_##name##_MASK/SHIFT.
> 
> Now we are not reading type or id from config. These macros are
> useless and incomplete.
> 
> So removing them for code cleaning.

Thanks, applied.

- Arnaldo

 
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>  tools/perf/util/parse-events.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index f05e15acd33f..3ed914882b96 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -149,14 +149,6 @@ struct event_symbol event_symbols_sw[PERF_COUNT_SW_MAX] = {
>  	},
>  };
>  
> -#define __PERF_EVENT_FIELD(config, name) \
> -	((config & PERF_EVENT_##name##_MASK) >> PERF_EVENT_##name##_SHIFT)
> -
> -#define PERF_EVENT_RAW(config)		__PERF_EVENT_FIELD(config, RAW)
> -#define PERF_EVENT_CONFIG(config)	__PERF_EVENT_FIELD(config, CONFIG)
> -#define PERF_EVENT_TYPE(config)		__PERF_EVENT_FIELD(config, TYPE)
> -#define PERF_EVENT_ID(config)		__PERF_EVENT_FIELD(config, EVENT)
> -
>  const char *event_type(int type)
>  {
>  	switch (type) {
> -- 
> 2.17.1

-- 

- Arnaldo

[PATCH -next 5/5] perf: Remove unused macro K

[Chen Zhongjin]

Unused macro reported by [-Wunused-macros].

This macro is introduced to calculate the 'unit' size, in:
'd2fb8b4151a9 ("perf tools: Add new perf_atoll() function to parse string representing size in bytes")'

'8ba7f6c2faad ("saner perf_atoll()")'
This commit has simplified the perf_atoll() function and remove the
'unit' variable. This macro is not deleted, but nowhere else is using it.

A single letter macro is confusing and easy to be misused.
So remove it for code cleaning.

Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 tools/perf/util/string.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/util/string.c b/tools/perf/util/string.c
index f6d90cdd9225..4f12a96f33cc 100644
--- a/tools/perf/util/string.c
+++ b/tools/perf/util/string.c
@@ -15,7 +15,6 @@ const char *dots =
 	"....................................................................."
 	".....................................................................";
 
-#define K 1024LL
 /*
  * perf_atoll()
  * Parse (\d+)(b|B|kb|KB|mb|MB|gb|GB|tb|TB) (e.g. "256MB")
-- 
2.17.1

Re: [PATCH -next 5/5] perf: Remove unused macro K

[Arnaldo Carvalho de Melo]

Em Mon, Sep 26, 2022 at 11:14:40AM +0800, Chen Zhongjin escreveu:
> Unused macro reported by [-Wunused-macros].
> 
> This macro is introduced to calculate the 'unit' size, in:
> 'd2fb8b4151a9 ("perf tools: Add new perf_atoll() function to parse string representing size in bytes")'
> 
> '8ba7f6c2faad ("saner perf_atoll()")'
> This commit has simplified the perf_atoll() function and remove the
> 'unit' variable. This macro is not deleted, but nowhere else is using it.
> 
> A single letter macro is confusing and easy to be misused.
> So remove it for code cleaning.

Thanks, applied.

- Arnaldo

 
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>  tools/perf/util/string.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tools/perf/util/string.c b/tools/perf/util/string.c
> index f6d90cdd9225..4f12a96f33cc 100644
> --- a/tools/perf/util/string.c
> +++ b/tools/perf/util/string.c
> @@ -15,7 +15,6 @@ const char *dots =
>  	"....................................................................."
>  	".....................................................................";
>  
> -#define K 1024LL
>  /*
>   * perf_atoll()
>   * Parse (\d+)(b|B|kb|KB|mb|MB|gb|GB|tb|TB) (e.g. "256MB")
> -- 
> 2.17.1

-- 

- Arnaldo