linux-kernel - Re: [PATCH v2 12/16] perf lock: Move common lock contention code to new file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fVvM_8T9b6EXC+JfpV5FAWPDJN=HfFtFhO+KU5rXd4P1Q@mail.gmail.com>
Date: Tue, 22 Oct 2024 09:56:02 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	James Clark <james.clark@...aro.org>, Howard Chu <howardchu95@...il.com>, 
	Athira Jajeev <atrajeev@...ux.vnet.ibm.com>, Michael Petlan <mpetlan@...hat.com>, 
	Veronika Molnarova <vmolnaro@...hat.com>, Dapeng Mi <dapeng1.mi@...ux.intel.com>, 
	Thomas Richter <tmricht@...ux.ibm.com>, Ilya Leoshkevich <iii@...ux.ibm.com>, 
	Colin Ian King <colin.i.king@...il.com>, Weilin Wang <weilin.wang@...el.com>, 
	Andi Kleen <ak@...ux.intel.com>, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v2 12/16] perf lock: Move common lock contention code to
 new file

On Mon, Oct 21, 2024 at 11:36 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Tue, Oct 15, 2024 at 09:24:11PM -0700, Ian Rogers wrote:
> > Avoid references from util code to builtin-lock that require python
> > stubs. Move the functions and related variables to
> > util/lock-contention.c. Add max_stack_depth parameter to
> > match_callstack_filter to avoid sharing a global variable.
> >
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > ---
> >  tools/perf/builtin-lock.c             | 137 +--------------------
> >  tools/perf/util/Build                 |   1 +
> >  tools/perf/util/bpf_lock_contention.c |   2 +-
> >  tools/perf/util/lock-contention.c     | 170 ++++++++++++++++++++++++++
> >  tools/perf/util/lock-contention.h     |  37 ++----
> >  tools/perf/util/python.c              |  17 ---
> >  6 files changed, 185 insertions(+), 179 deletions(-)
> >  create mode 100644 tools/perf/util/lock-contention.c
> >
> [SNIP]
> > diff --git a/tools/perf/util/Build b/tools/perf/util/Build
> > index 2a2f7780e595..ea2d9eced92e 100644
> > --- a/tools/perf/util/Build
> > +++ b/tools/perf/util/Build
> > @@ -121,6 +121,7 @@ perf-util-y += topdown.o
> >  perf-util-y += iostat.o
> >  perf-util-y += stream.o
> >  perf-util-y += kvm-stat.o
> > +perf-util-y += lock-contention.o
> >  perf-util-$(CONFIG_AUXTRACE) += auxtrace.o
> >  perf-util-$(CONFIG_AUXTRACE) += intel-pt-decoder/
> >  perf-util-$(CONFIG_AUXTRACE) += intel-pt.o
> > diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c
> > index 41a1ad087895..37e17c56f106 100644
> > --- a/tools/perf/util/bpf_lock_contention.c
> > +++ b/tools/perf/util/bpf_lock_contention.c
> > @@ -458,7 +458,7 @@ int lock_contention_read(struct lock_contention *con)
> >               if (con->save_callstack) {
> >                       bpf_map_lookup_elem(stack, &key.stack_id, stack_trace);
> >
> > -                     if (!match_callstack_filter(machine, stack_trace)) {
> > +                     if (!match_callstack_filter(machine, stack_trace, con->max_stack)) {
> >                               con->nr_filtered += data.count;
> >                               goto next;
> >                       }
> > diff --git a/tools/perf/util/lock-contention.c b/tools/perf/util/lock-contention.c
> > new file mode 100644
> > index 000000000000..841bb18b1f06
> > --- /dev/null
> > +++ b/tools/perf/util/lock-contention.c
> [SNIP]
> > +#ifndef HAVE_BPF_SKEL
> > +int lock_contention_prepare(struct lock_contention *con __maybe_unused)
> > +{
> > +     return 0;
> > +}
> > +
> > +int lock_contention_start(void)
> > +{
> > +     return 0;
> > +}
> > +
> > +int lock_contention_stop(void)
> > +{
> > +     return 0;
> > +}
> > +
> > +int lock_contention_finish(struct lock_contention *con __maybe_unused)
> > +{
> > +     return 0;
> > +}
> > +
> > +int lock_contention_read(struct lock_contention *con __maybe_unused)
> > +{
> > +     return 0;
> > +}
> > +#endif  /* !HAVE_BPF_SKEL */
>
> Do we really need this part?  Why not leave it in the header file?

I agree it could be in the header but I think it is better not to have
it in the header. The change is trying to move things into a new
util/lock-contention.c. We didn't have the C file in the past so
putting things in the header was okay. In general `static inline` is
something of a code smell, the static says make it local to every C
file leading to duplication, the inline doesn't actually mean inline
the code it means the symbol definition should be hidden. There's
generally plenty of confusion over static inline that it's best to
avoid its use, we don't want code in header files and let LTO resolve
performance issues - there are different arguments in particular
performance crucial code that relies on inlining, but this isn't it.
It also helps the debugger to place the code into the C file. There is
also a lessening of cognitive load when you read the header file which
is smaller and without #ifdefs. I was also hoping to reduce/remove the
scope of these functions if possible.

So yes, strictly this tidying isn't necessary but given I was creating
the C file and tidying lock_contention.h it made sense to me to clean
up these.

Thanks,
Ian