[PATCH 0/4] perf: Some clean up

[PATCH 0/4] perf: Some clean up

[Shang XiaoJing]

Some clean ups in tools/perf.

Shang XiaoJing (4):
  perf genelf: Fix error code in jit_write_elf()
  perf stat: Merge cases in process_evlist
  perf top: Fix error code in cmd_top()
  perf stat: Clean redundant if in process_evlist

 tools/perf/builtin-stat.c | 5 -----
 tools/perf/builtin-top.c  | 3 +++
 tools/perf/util/genelf.c  | 1 +
 3 files changed, 4 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH 1/4] perf genelf: Fix error code in jit_write_elf()

[Shang XiaoJing]

The error code is set to -1 at the beginning of jit_write_elf(), but it is
assigned by jit_add_eh_frame_info() in the middle, hence the following
error can only return the error code of jit_add_eh_frame_info(). Reset
the error code to the default value after being assigned by
jit_add_eh_frame_info().

Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
---
 tools/perf/util/genelf.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
index ed28a0dbcb7f..2e45b204494a 100644
--- a/tools/perf/util/genelf.c
+++ b/tools/perf/util/genelf.c
@@ -331,6 +331,7 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
 					       eh_frame_base_offset);
 		if (retval)
 			goto error;
+		retval = -1;
 	}
 
 	/*
-- 
2.17.1

Re: [PATCH 1/4] perf genelf: Fix error code in jit_write_elf()

[Arnaldo Carvalho de Melo]

Em Thu, Sep 22, 2022 at 10:14:35PM +0800, Shang XiaoJing escreveu:
> The error code is set to -1 at the beginning of jit_write_elf(), but it is
> assigned by jit_add_eh_frame_info() in the middle, hence the following
> error can only return the error code of jit_add_eh_frame_info(). Reset
> the error code to the default value after being assigned by
> jit_add_eh_frame_info().

You forgot to add:

Cc: Stefano Sanfilippo <ssanfilippo@chromium.org>
Fixes: 086f9f3d7897d808 ("perf jit: Generate .eh_frame/.eh_frame_hdr in DSO")

Applied.

- Arnaldo
 
> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
> ---
>  tools/perf/util/genelf.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/perf/util/genelf.c b/tools/perf/util/genelf.c
> index ed28a0dbcb7f..2e45b204494a 100644
> --- a/tools/perf/util/genelf.c
> +++ b/tools/perf/util/genelf.c
> @@ -331,6 +331,7 @@ jit_write_elf(int fd, uint64_t load_addr, const char *sym,
>  					       eh_frame_base_offset);
>  		if (retval)
>  			goto error;
> +		retval = -1;
>  	}
>  
>  	/*
> -- 
> 2.17.1

-- 

- Arnaldo

[PATCH 2/4] perf stat: Merge cases in process_evlist

[Shang XiaoJing]

As two cases in process_evlist has same behavior, merge these cases.

Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
---
 tools/perf/builtin-stat.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index e05fe72c1d87..e10595f649bc 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -662,9 +662,6 @@ static void process_evlist(struct evlist *evlist, unsigned int interval)
 	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
 		switch (cmd) {
 		case EVLIST_CTL_CMD_ENABLE:
-			if (interval)
-				process_interval();
-			break;
 		case EVLIST_CTL_CMD_DISABLE:
 			if (interval)
 				process_interval();
-- 
2.17.1

Re: [PATCH 2/4] perf stat: Merge cases in process_evlist

[Arnaldo Carvalho de Melo]

Em Thu, Sep 22, 2022 at 10:14:36PM +0800, Shang XiaoJing escreveu:
> As two cases in process_evlist has same behavior, merge these cases.

Applied.

Added the patch below, see:

include/linux/compiler_attributes.h and
tools/include/linux/compiler-gcc.h.

- Arnaldo

Commiter notes:

Added __fallthrough, the kernel has "fallthrough", we need to make
tools/ use it.

---

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index e10595f649bcb2b4..7b8e901bce101b63 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -662,6 +662,7 @@ static void process_evlist(struct evlist *evlist, unsigned int interval)
 	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
 		switch (cmd) {
 		case EVLIST_CTL_CMD_ENABLE:
+			__fallthrough;
 		case EVLIST_CTL_CMD_DISABLE:
 			if (interval)
 				process_interval();
 
> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
> ---
>  tools/perf/builtin-stat.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index e05fe72c1d87..e10595f649bc 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -662,9 +662,6 @@ static void process_evlist(struct evlist *evlist, unsigned int interval)
>  	if (evlist__ctlfd_process(evlist, &cmd) > 0) {
>  		switch (cmd) {
>  		case EVLIST_CTL_CMD_ENABLE:
> -			if (interval)
> -				process_interval();
> -			break;
>  		case EVLIST_CTL_CMD_DISABLE:
>  			if (interval)
>  				process_interval();
> -- 
> 2.17.1

-- 

- Arnaldo

[PATCH 3/4] perf top: Fix error code in cmd_top()

[Shang XiaoJing]

There are three error paths which return success:

1. Propagate the errno from evlist__create_maps() if it failed.

2. Return -EINVAL if top.sb_evlist is NULL.

3. Return -EINVAL if evlist__add_bpf_sb_event() failed.

Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
---
 tools/perf/builtin-top.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index e89208b4ad4b..4b3ff7687236 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1707,6 +1707,7 @@ int cmd_top(int argc, const char **argv)
 	if (evlist__create_maps(top.evlist, target) < 0) {
 		ui__error("Couldn't create thread/CPU maps: %s\n",
 			  errno == ENOENT ? "No such process" : str_error_r(errno, errbuf, sizeof(errbuf)));
+		status = -errno;
 		goto out_delete_evlist;
 	}
 
@@ -1759,11 +1760,13 @@ int cmd_top(int argc, const char **argv)
 
 		if (top.sb_evlist == NULL) {
 			pr_err("Couldn't create side band evlist.\n.");
+			status = -EINVAL;
 			goto out_delete_evlist;
 		}
 
 		if (evlist__add_bpf_sb_event(top.sb_evlist, &perf_env)) {
 			pr_err("Couldn't ask for PERF_RECORD_BPF_EVENT side band events.\n.");
+			status = -EINVAL;
 			goto out_delete_evlist;
 		}
 	}
-- 
2.17.1

[PATCH 4/4] perf stat: Clean redundant if in process_evlist

[Shang XiaoJing]

Since the first if statment is covered by the following one, clean up
the first if statment.

Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
---
 tools/perf/builtin-stat.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index e10595f649bc..ccea5d1e053a 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -899,8 +899,6 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 		evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
 			counter = evlist_cpu_itr.evsel;
 
-			if (!counter->reset_group && !counter->errored)
-				continue;
 			if (!counter->reset_group)
 				continue;
 try_again_reset:
-- 
2.17.1

Re: [PATCH 4/4] perf stat: Clean redundant if in process_evlist

[Arnaldo Carvalho de Melo]

Em Thu, Sep 22, 2022 at 10:14:38PM +0800, Shang XiaoJing escreveu:
> Since the first if statment is covered by the following one, clean up
> the first if statment.
> 
> Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
> ---
>  tools/perf/builtin-stat.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index e10595f649bc..ccea5d1e053a 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -899,8 +899,6 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
>  		evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
>  			counter = evlist_cpu_itr.evsel;
>  
> -			if (!counter->reset_group && !counter->errored)
> -				continue;
>  			if (!counter->reset_group)
>  				continue;

Ian, can you please take a look at this one? You added this in:

commit 472832d2c000b9611feaea66fe521055c3dbf17a
Author: Ian Rogers <irogers@google.com>
Date:   Tue Jan 4 22:13:37 2022 -0800

    perf evlist: Refactor evlist__for_each_cpu()

>  try_again_reset:
> -- 
> 2.17.1


- Arnaldo

Re: [PATCH 4/4] perf stat: Clean redundant if in process_evlist

[Ian Rogers]

On Thu, Sep 22, 2022 at 12:55 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Thu, Sep 22, 2022 at 10:14:38PM +0800, Shang XiaoJing escreveu:
> > Since the first if statment is covered by the following one, clean up
> > the first if statment.
> >
> > Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> > ---
> >  tools/perf/builtin-stat.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index e10595f649bc..ccea5d1e053a 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -899,8 +899,6 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> >               evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
> >                       counter = evlist_cpu_itr.evsel;
> >
> > -                     if (!counter->reset_group && !counter->errored)
> > -                             continue;
> >                       if (!counter->reset_group)
> >                               continue;
>
> Ian, can you please take a look at this one? You added this in:
>
> commit 472832d2c000b9611feaea66fe521055c3dbf17a
> Author: Ian Rogers <irogers@google.com>
> Date:   Tue Jan 4 22:13:37 2022 -0800
>
>     perf evlist: Refactor evlist__for_each_cpu()
>
> >  try_again_reset:
> > --
> > 2.17.1
>
>
> - Arnaldo

Re: [PATCH 4/4] perf stat: Clean redundant if in process_evlist

[Arnaldo Carvalho de Melo]

Em Thu, Sep 22, 2022 at 03:50:40PM -0700, Ian Rogers escreveu:
> On Thu, Sep 22, 2022 at 12:55 PM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Thu, Sep 22, 2022 at 10:14:38PM +0800, Shang XiaoJing escreveu:
> > > Since the first if statment is covered by the following one, clean up
> > > the first if statment.
> > >
> > > Signed-off-by: Shang XiaoJing <shangxiaojing@huawei.com>
> 
> Acked-by: Ian Rogers <irogers@google.com>

Thanks, applied.

- Arnaldo

 
> Thanks,
> Ian
> 
> > > ---
> > >  tools/perf/builtin-stat.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > > index e10595f649bc..ccea5d1e053a 100644
> > > --- a/tools/perf/builtin-stat.c
> > > +++ b/tools/perf/builtin-stat.c
> > > @@ -899,8 +899,6 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> > >               evlist__for_each_cpu(evlist_cpu_itr, evsel_list, affinity) {
> > >                       counter = evlist_cpu_itr.evsel;
> > >
> > > -                     if (!counter->reset_group && !counter->errored)
> > > -                             continue;
> > >                       if (!counter->reset_group)
> > >                               continue;
> >
> > Ian, can you please take a look at this one? You added this in:
> >
> > commit 472832d2c000b9611feaea66fe521055c3dbf17a
> > Author: Ian Rogers <irogers@google.com>
> > Date:   Tue Jan 4 22:13:37 2022 -0800
> >
> >     perf evlist: Refactor evlist__for_each_cpu()
> >
> > >  try_again_reset:
> > > --
> > > 2.17.1
> >
> >
> > - Arnaldo

-- 

- Arnaldo