2022-09-08 01:45:56

by Shang XiaoJing

[permalink] [raw]
Subject: [PATCH 0/4] perf: Clean up by adding helpers

Some clean up in builtin-lock.c, builtin-timechart.c, and
builtin-trace.c.

Shang XiaoJing (4):
perf trace: Use zalloc to save initialization of syscall_stats
perf lock: Add get_key_by_aggr_mode helper
perf timechart: Add create_pidcomm helper
perf timechart: Add p_state_end helper

tools/perf/builtin-lock.c | 129 ++++++++++++++-------------------
tools/perf/builtin-timechart.c | 65 +++++++++--------
tools/perf/builtin-trace.c | 5 +-
3 files changed, 88 insertions(+), 111 deletions(-)

--
2.17.1


2022-09-08 01:56:14

by Shang XiaoJing

[permalink] [raw]
Subject: [PATCH 3/4] perf timechart: Add create_pidcomm helper

Wrap repeated code combined with alloc of per_pidcomm in helper function
create_pidcomm.

Signed-off-by: Shang XiaoJing <[email protected]>
---
tools/perf/builtin-timechart.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/tools/perf/builtin-timechart.c b/tools/perf/builtin-timechart.c
index e2e9ad929baf..667a94d45493 100644
--- a/tools/perf/builtin-timechart.c
+++ b/tools/perf/builtin-timechart.c
@@ -215,6 +215,19 @@ static struct per_pid *find_create_pid(struct timechart *tchart, int pid)
return cursor;
}

+static struct per_pidcomm *create_pidcomm(struct per_pid *p)
+{
+ struct per_pidcomm *c;
+
+ c = zalloc(sizeof(*c));
+ if (!c)
+ return NULL;
+ p->current = c;
+ c->next = p->all;
+ p->all = c;
+ return c;
+}
+
static void pid_set_comm(struct timechart *tchart, int pid, char *comm)
{
struct per_pid *p;
@@ -233,12 +246,9 @@ static void pid_set_comm(struct timechart *tchart, int pid, char *comm)
}
c = c->next;
}
- c = zalloc(sizeof(*c));
+ c = create_pidcomm(p);
assert(c != NULL);
c->comm = strdup(comm);
- p->current = c;
- c->next = p->all;
- p->all = c;
}

static void pid_fork(struct timechart *tchart, int pid, int ppid, u64 timestamp)
@@ -277,11 +287,8 @@ static void pid_put_sample(struct timechart *tchart, int pid, int type,
p = find_create_pid(tchart, pid);
c = p->current;
if (!c) {
- c = zalloc(sizeof(*c));
+ c = create_pidcomm(p);
assert(c != NULL);
- p->current = c;
- c->next = p->all;
- p->all = c;
}

sample = zalloc(sizeof(*sample));
@@ -726,12 +733,9 @@ static int pid_begin_io_sample(struct timechart *tchart, int pid, int type,
struct io_sample *prev;

if (!c) {
- c = zalloc(sizeof(*c));
+ c = create_pidcomm(p);
if (!c)
return -ENOMEM;
- p->current = c;
- c->next = p->all;
- p->all = c;
}

prev = c->io_samples;
--
2.17.1

2022-09-08 02:23:25

by Shang XiaoJing

[permalink] [raw]
Subject: [PATCH 1/4] perf trace: Use zalloc to save initialization of syscall_stats

As most members of syscall_stats is set to 0 in thread__update_stats,
using zalloc directly.

Signed-off-by: Shang XiaoJing <[email protected]>
---
tools/perf/builtin-trace.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 0bd9d01c0df9..3ecc31375f90 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -2173,13 +2173,10 @@ static void thread__update_stats(struct thread *thread, struct thread_trace *ttr

stats = inode->priv;
if (stats == NULL) {
- stats = malloc(sizeof(*stats));
+ stats = zalloc(sizeof(*stats));
if (stats == NULL)
return;

- stats->nr_failures = 0;
- stats->max_errno = 0;
- stats->errnos = NULL;
init_stats(&stats->stats);
inode->priv = stats;
}
--
2.17.1

2022-09-08 02:26:53

by Shang XiaoJing

[permalink] [raw]
Subject: [PATCH 2/4] perf lock: Add get_key_by_aggr_mode helper

Wrap repeated code in helper functions get_key_by_aggr_mode and
get_key_by_aggr_mode_simple, which assign the value to key based on
aggregation mode. Note that for the conditions not support
LOCK_AGGR_CALLER, should call get_key_by_aggr_mode_simple directly.

Signed-off-by: Shang XiaoJing <[email protected]>
---
tools/perf/builtin-lock.c | 129 ++++++++++++++++----------------------
1 file changed, 53 insertions(+), 76 deletions(-)

diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 70197c0593b1..44a47648b7fe 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -560,29 +560,50 @@ enum acquire_flags {
READ_LOCK = 2,
};

-static int report_lock_acquire_event(struct evsel *evsel,
- struct perf_sample *sample)
+static int get_key_by_aggr_mode_simple(u64 *key, u64 addr, u32 tid)
{
- struct lock_stat *ls;
- struct thread_stat *ts;
- struct lock_seq_stat *seq;
- const char *name = evsel__strval(evsel, sample, "name");
- u64 addr = evsel__intval(evsel, sample, "lockdep_addr");
- int flag = evsel__intval(evsel, sample, "flags");
- u64 key;
-
switch (aggr_mode) {
case LOCK_AGGR_ADDR:
- key = addr;
+ *key = addr;
break;
case LOCK_AGGR_TASK:
- key = sample->tid;
+ *key = tid;
break;
case LOCK_AGGR_CALLER:
default:
pr_err("Invalid aggregation mode: %d\n", aggr_mode);
return -EINVAL;
}
+ return 0;
+}
+
+static u64 callchain_id(struct evsel *evsel, struct perf_sample *sample);
+
+static int get_key_by_aggr_mode(u64 *key, u64 addr, struct evsel *evsel,
+ struct perf_sample *sample)
+{
+ if (aggr_mode == LOCK_AGGR_CALLER) {
+ *key = callchain_id(evsel, sample);
+ return 0;
+ }
+ return get_key_by_aggr_mode_simple(key, addr, sample->tid);
+}
+
+static int report_lock_acquire_event(struct evsel *evsel,
+ struct perf_sample *sample)
+{
+ struct lock_stat *ls;
+ struct thread_stat *ts;
+ struct lock_seq_stat *seq;
+ const char *name = evsel__strval(evsel, sample, "name");
+ u64 addr = evsel__intval(evsel, sample, "lockdep_addr");
+ int flag = evsel__intval(evsel, sample, "flags");
+ u64 key;
+ int ret;
+
+ ret = get_key_by_aggr_mode_simple(&key, addr, sample->tid);
+ if (ret < 0)
+ return ret;

ls = lock_stat_findnew(key, name, 0);
if (!ls)
@@ -653,19 +674,11 @@ static int report_lock_acquired_event(struct evsel *evsel,
const char *name = evsel__strval(evsel, sample, "name");
u64 addr = evsel__intval(evsel, sample, "lockdep_addr");
u64 key;
+ int ret;

- switch (aggr_mode) {
- case LOCK_AGGR_ADDR:
- key = addr;
- break;
- case LOCK_AGGR_TASK:
- key = sample->tid;
- break;
- case LOCK_AGGR_CALLER:
- default:
- pr_err("Invalid aggregation mode: %d\n", aggr_mode);
- return -EINVAL;
- }
+ ret = get_key_by_aggr_mode_simple(&key, addr, sample->tid);
+ if (ret < 0)
+ return ret;

ls = lock_stat_findnew(key, name, 0);
if (!ls)
@@ -726,19 +739,11 @@ static int report_lock_contended_event(struct evsel *evsel,
const char *name = evsel__strval(evsel, sample, "name");
u64 addr = evsel__intval(evsel, sample, "lockdep_addr");
u64 key;
+ int ret;

- switch (aggr_mode) {
- case LOCK_AGGR_ADDR:
- key = addr;
- break;
- case LOCK_AGGR_TASK:
- key = sample->tid;
- break;
- case LOCK_AGGR_CALLER:
- default:
- pr_err("Invalid aggregation mode: %d\n", aggr_mode);
- return -EINVAL;
- }
+ ret = get_key_by_aggr_mode_simple(&key, addr, sample->tid);
+ if (ret < 0)
+ return ret;

ls = lock_stat_findnew(key, name, 0);
if (!ls)
@@ -792,19 +797,11 @@ static int report_lock_release_event(struct evsel *evsel,
const char *name = evsel__strval(evsel, sample, "name");
u64 addr = evsel__intval(evsel, sample, "lockdep_addr");
u64 key;
+ int ret;

- switch (aggr_mode) {
- case LOCK_AGGR_ADDR:
- key = addr;
- break;
- case LOCK_AGGR_TASK:
- key = sample->tid;
- break;
- case LOCK_AGGR_CALLER:
- default:
- pr_err("Invalid aggregation mode: %d\n", aggr_mode);
- return -EINVAL;
- }
+ ret = get_key_by_aggr_mode_simple(&key, addr, sample->tid);
+ if (ret < 0)
+ return ret;

ls = lock_stat_findnew(key, name, 0);
if (!ls)
@@ -1015,21 +1012,11 @@ static int report_lock_contention_begin_event(struct evsel *evsel,
struct lock_seq_stat *seq;
u64 addr = evsel__intval(evsel, sample, "lock_addr");
u64 key;
+ int ret;

- switch (aggr_mode) {
- case LOCK_AGGR_ADDR:
- key = addr;
- break;
- case LOCK_AGGR_TASK:
- key = sample->tid;
- break;
- case LOCK_AGGR_CALLER:
- key = callchain_id(evsel, sample);
- break;
- default:
- pr_err("Invalid aggregation mode: %d\n", aggr_mode);
- return -EINVAL;
- }
+ ret = get_key_by_aggr_mode(&key, addr, evsel, sample);
+ if (ret < 0)
+ return ret;

ls = lock_stat_find(key);
if (!ls) {
@@ -1098,21 +1085,11 @@ static int report_lock_contention_end_event(struct evsel *evsel,
u64 contended_term;
u64 addr = evsel__intval(evsel, sample, "lock_addr");
u64 key;
+ int ret;

- switch (aggr_mode) {
- case LOCK_AGGR_ADDR:
- key = addr;
- break;
- case LOCK_AGGR_TASK:
- key = sample->tid;
- break;
- case LOCK_AGGR_CALLER:
- key = callchain_id(evsel, sample);
- break;
- default:
- pr_err("Invalid aggregation mode: %d\n", aggr_mode);
- return -EINVAL;
- }
+ ret = get_key_by_aggr_mode(&key, addr, evsel, sample);
+ if (ret < 0)
+ return ret;

ls = lock_stat_find(key);
if (!ls)
--
2.17.1

2022-09-08 08:04:39

by Namhyung Kim

[permalink] [raw]
Subject: Re: [PATCH 0/4] perf: Clean up by adding helpers

Hello,

On Wed, Sep 7, 2022 at 6:37 PM Shang XiaoJing <[email protected]> wrote:
>
> Some clean up in builtin-lock.c, builtin-timechart.c, and
> builtin-trace.c.
>
> Shang XiaoJing (4):
> perf trace: Use zalloc to save initialization of syscall_stats
> perf lock: Add get_key_by_aggr_mode helper
> perf timechart: Add create_pidcomm helper
> perf timechart: Add p_state_end helper

Acked-by: Namhyung Kim <[email protected]>

Thanks,
Namhyung


>
> tools/perf/builtin-lock.c | 129 ++++++++++++++-------------------
> tools/perf/builtin-timechart.c | 65 +++++++++--------
> tools/perf/builtin-trace.c | 5 +-
> 3 files changed, 88 insertions(+), 111 deletions(-)
>
> --
> 2.17.1
>

2022-09-08 19:11:18

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 0/4] perf: Clean up by adding helpers

Em Thu, Sep 08, 2022 at 12:08:06AM -0700, Namhyung Kim escreveu:
> Hello,
>
> On Wed, Sep 7, 2022 at 6:37 PM Shang XiaoJing <[email protected]> wrote:
> >
> > Some clean up in builtin-lock.c, builtin-timechart.c, and
> > builtin-trace.c.
> >
> > Shang XiaoJing (4):
> > perf trace: Use zalloc to save initialization of syscall_stats
> > perf lock: Add get_key_by_aggr_mode helper
> > perf timechart: Add create_pidcomm helper
> > perf timechart: Add p_state_end helper
>
> Acked-by: Namhyung Kim <[email protected]>

Thanks, applied.

- Arnaldo


> Thanks,
> Namhyung
>
>
> >
> > tools/perf/builtin-lock.c | 129 ++++++++++++++-------------------
> > tools/perf/builtin-timechart.c | 65 +++++++++--------
> > tools/perf/builtin-trace.c | 5 +-
> > 3 files changed, 88 insertions(+), 111 deletions(-)
> >
> > --
> > 2.17.1
> >

--

- Arnaldo