From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>, Ingo Molnar <mingo@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
LKML <linux-kernel@vger.kernel.org>,
Ian Rogers <irogers@google.com>,
linux-perf-users@vger.kernel.org, Will Deacon <will@kernel.org>,
Waiman Long <longman@redhat.com>,
Boqun Feng <boqun.feng@gmail.com>,
Davidlohr Bueso <dave@stgolabs.net>
Subject: Re: [PATCH 3/6] perf lock: Add lock aggregation enum
Date: Thu, 21 Jul 2022 16:05:52 -0300 [thread overview]
Message-ID: <YtmjkCbvemjvlPBN@kernel.org> (raw)
In-Reply-To: <Ytmi4otIpXA+zNSx@kernel.org>
Em Thu, Jul 21, 2022 at 04:02:58PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Jul 20, 2022 at 09:36:41PM -0700, Namhyung Kim escreveu:
> > Introduce the aggr_mode variable to prepare the later code change.
> > The default is LOCK_AGGR_ADDR which aggregate the result for the lock
> > instances. When -t/--threads option is given, it'd be set to
> > LOCK_AGGR_TASK. The LOCK_AGGR_CALLER is for the contention analysis
> > and it'd aggregate the stat by comparing the callstacks.
>
> builtin-lock.c: In function ‘report_lock_contention_end_event’:
> builtin-lock.c:1113:13: error: variable ‘key’ set but not used [-Werror=unused-but-set-variable]
> 1113 | u64 key;
> | ^~~
> cc1: all warnings being treated as errors
>
>
> trying to fix
Please take a look at this:
builtin-lock.c: In function ‘report_lock_contention_end_event’:
builtin-lock.c:1113:13: error: variable ‘key’ set but not used [-Werror=unused-but-set-variable]
1113 | u64 key;
| ^~~
cc1: all warnings being treated as errors
make[3]: *** [/var/home/acme/git/perf/tools/build/Makefile.build:96: /tmp/build/perf/builtin-lock.o] Error 1
make[2]: *** [Makefile.perf:660: /tmp/build/perf/perf-in.o] Error 2
make[1]: *** [Makefile.perf:240: sub-make] Error 2
make: *** [Makefile:113: install-bin] Error 2
make: Leaving directory '/var/home/acme/git/perf/tools/perf'
Performance counter stats for 'make -k BUILD_BPF_SKEL=1 CORESIGHT=1 PYTHON=python3 O=/tmp/build/perf -C tools/perf install-bin':
6,642,782,729 cycles:u
12,246,940,928 instructions:u # 1.84 insn per cycle
2.513486078 seconds time elapsed
1.649124000 seconds user
1.272055000 seconds sys
⬢[acme@toolbox perf]$ gcc -v
Using built-in specs.
COLLECT_GCC=/usr/bin/gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/12/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-redhat-linux
Configured with: ../configure --enable-bootstrap --enable-languages=c,c++,fortran,objc,obj-c++,ada,go,d,lto --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-shared --enable-threads=posix --enable-checking=release --enable-multilib --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-gcc-major-version-only --enable-libstdcxx-backtrace --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl=/builddir/build/BUILD/gcc-12.1.1-20220507/obj-x86_64-redhat-linux/isl-install --enable-offload-targets=nvptx-none --without-cuda-driver --enable-offload-defaulted --enable-gnu-indirect-function --enable-cet --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux --with-build-config=bootstrap-lto --enable-link-serialization=1
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 12.1.1 20220507 (Red Hat 12.1.1-1) (GCC)
⬢[acme@toolbox perf]$ cat /etc/fedora-release
Fedora release 36 (Thirty Six)
⬢[acme@toolbox perf]$
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > tools/perf/builtin-lock.c | 112 +++++++++++++++++++++++++++++++-------
> > 1 file changed, 93 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> > index 1de459198b99..551bad905139 100644
> > --- a/tools/perf/builtin-lock.c
> > +++ b/tools/perf/builtin-lock.c
> > @@ -126,6 +126,12 @@ static struct rb_root thread_stats;
> > static bool combine_locks;
> > static bool show_thread_stats;
> >
> > +static enum {
> > + LOCK_AGGR_ADDR,
> > + LOCK_AGGR_TASK,
> > + LOCK_AGGR_CALLER,
> > +} aggr_mode = LOCK_AGGR_ADDR;
> > +
> > /*
> > * CONTENTION_STACK_DEPTH
> > * Number of stack trace entries to find callers
> > @@ -622,12 +628,22 @@ static int report_lock_acquire_event(struct evsel *evsel,
> > 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;
> >
> > - /* abuse ls->addr for tid */
> > - if (show_thread_stats)
> > - addr = sample->tid;
> > + 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;
> > + }
> >
> > - ls = lock_stat_findnew(addr, name, 0);
> > + ls = lock_stat_findnew(key, name, 0);
> > if (!ls)
> > return -ENOMEM;
> >
> > @@ -695,11 +711,22 @@ static int report_lock_acquired_event(struct evsel *evsel,
> > u64 contended_term;
> > const char *name = evsel__strval(evsel, sample, "name");
> > u64 addr = evsel__intval(evsel, sample, "lockdep_addr");
> > + u64 key;
> >
> > - if (show_thread_stats)
> > - addr = sample->tid;
> > + 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;
> > + }
> >
> > - ls = lock_stat_findnew(addr, name, 0);
> > + ls = lock_stat_findnew(key, name, 0);
> > if (!ls)
> > return -ENOMEM;
> >
> > @@ -757,11 +784,22 @@ static int report_lock_contended_event(struct evsel *evsel,
> > struct lock_seq_stat *seq;
> > const char *name = evsel__strval(evsel, sample, "name");
> > u64 addr = evsel__intval(evsel, sample, "lockdep_addr");
> > + u64 key;
> >
> > - if (show_thread_stats)
> > - addr = sample->tid;
> > + 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;
> > + }
> >
> > - ls = lock_stat_findnew(addr, name, 0);
> > + ls = lock_stat_findnew(key, name, 0);
> > if (!ls)
> > return -ENOMEM;
> >
> > @@ -812,11 +850,22 @@ static int report_lock_release_event(struct evsel *evsel,
> > struct lock_seq_stat *seq;
> > const char *name = evsel__strval(evsel, sample, "name");
> > u64 addr = evsel__intval(evsel, sample, "lockdep_addr");
> > + u64 key;
> >
> > - if (show_thread_stats)
> > - addr = sample->tid;
> > + 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;
> > + }
> >
> > - ls = lock_stat_findnew(addr, name, 0);
> > + ls = lock_stat_findnew(key, name, 0);
> > if (!ls)
> > return -ENOMEM;
> >
> > @@ -980,11 +1029,22 @@ static int report_lock_contention_begin_event(struct evsel *evsel,
> > struct thread_stat *ts;
> > struct lock_seq_stat *seq;
> > u64 addr = evsel__intval(evsel, sample, "lock_addr");
> > + u64 key;
> >
> > - if (show_thread_stats)
> > - addr = sample->tid;
> > + 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;
> > + }
> >
> > - ls = lock_stat_find(addr);
> > + ls = lock_stat_find(key);
> > if (!ls) {
> > char buf[128];
> > const char *caller = buf;
> > @@ -993,7 +1053,7 @@ static int report_lock_contention_begin_event(struct evsel *evsel,
> > if (lock_contention_caller(evsel, sample, buf, sizeof(buf)) < 0)
> > caller = "Unknown";
> >
> > - ls = lock_stat_findnew(addr, caller, flags);
> > + ls = lock_stat_findnew(key, caller, flags);
> > if (!ls)
> > return -ENOMEM;
> > }
> > @@ -1050,9 +1110,20 @@ static int report_lock_contention_end_event(struct evsel *evsel,
> > struct lock_seq_stat *seq;
> > u64 contended_term;
> > u64 addr = evsel__intval(evsel, sample, "lock_addr");
> > + u64 key;
> >
> > - if (show_thread_stats)
> > - addr = sample->tid;
> > + 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;
> > + }
> >
> > ls = lock_stat_find(addr);
> > if (!ls)
> > @@ -1416,6 +1487,9 @@ static int __cmd_report(bool display_info)
> > if (select_key())
> > goto out_delete;
> >
> > + if (show_thread_stats)
> > + aggr_mode = LOCK_AGGR_TASK;
> > +
> > err = perf_session__process_events(session);
> > if (err)
> > goto out_delete;
> > --
> > 2.37.0.170.g444d1eabd0-goog
>
> --
>
> - Arnaldo
--
- Arnaldo
next prev parent reply other threads:[~2022-07-21 19:06 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-21 4:36 [PATCHSET 0/6] perf lock: Add contention subcommand (v1) Namhyung Kim
2022-07-21 4:36 ` [PATCH 1/6] perf lock: Fix a copy-n-paste bug Namhyung Kim
2022-07-21 19:09 ` Arnaldo Carvalho de Melo
2022-07-21 4:36 ` [PATCH 2/6] perf lock: Add flags field in the lock_stat Namhyung Kim
2022-07-21 4:36 ` [PATCH 3/6] perf lock: Add lock aggregation enum Namhyung Kim
2022-07-21 19:02 ` Arnaldo Carvalho de Melo
2022-07-21 19:05 ` Arnaldo Carvalho de Melo [this message]
2022-07-25 16:56 ` Namhyung Kim
2022-07-21 4:36 ` [PATCH 4/6] perf lock: Add 'contention' subcommand Namhyung Kim
2022-07-21 4:36 ` [PATCH 5/6] perf lock: Add -k and -F options to " Namhyung Kim
2022-07-21 4:36 ` [PATCH 6/6] perf lock: Support -t option for " Namhyung Kim
2022-08-03 9:51 ` [PATCHSET 0/6] perf lock: Add contention subcommand (v1) Ingo Molnar
2022-08-04 4:55 ` Namhyung Kim
2022-08-04 8:37 ` Ingo Molnar
2022-08-04 21:02 ` Namhyung Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YtmjkCbvemjvlPBN@kernel.org \
--to=acme@kernel.org \
--cc=boqun.feng@gmail.com \
--cc=dave@stgolabs.net \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=longman@redhat.com \
--cc=mingo@kernel.org \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.