linux-kernel - Re: [PATCH v6 15/22] 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] [day] [month] [year] [list]

Message-ID: <CAP-5=fWiL0iBLue0fbCR3zgrCrX8j09ZSk5nkDr4pBsRGJUY0A@mail.gmail.com>
Date: Tue, 19 Nov 2024 09:21:46 -0800
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>, Josh Poimboeuf <jpoimboe@...hat.com>, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org, Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH v6 15/22] perf lock: Move common lock contention code to
 new file

On Tue, Nov 19, 2024 at 9:00 AM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Mon, Nov 18, 2024 at 05:03:41PM -0800, Ian Rogers wrote:
> > On Mon, Nov 18, 2024 at 4:23 PM Namhyung Kim <namhyung@...nel.org> wrote:
> [SNIP]
> > > On Fri, Nov 08, 2024 at 10:18:02PM -0800, Ian Rogers wrote:
> > > > +#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 */
> > >
> > > I still think it's the convention to have them in a header file as
> > > static inline functions and reduce the #ifdef in the .c file.
> >
> > Shouldn't minimizing ifdefs, and associated cognitive load, in header
> > files be the priority given they are #included many times while the .c
> > file is only compiled once?
> > Shouldn't a goal of the header file be to abstract away things like
> > HAVE_BPF_SKEL?
> > I'm not clear what the goal of having the functions in the header
> > files is, performance? The code isn't going to run anyway. I feel
> > lock_contention.h is smaller and easier to read like this but I also
> > don't care enough to fight. I did this change here as
> > lock_contention.h was being brought into python.c for the sake of
> > stubbing out functions that the header file was also subbing out for
> > !BPF_HAVE_SKEL. A single stub felt like progress.
>
> I think it may have the empty functions in the binary if we keep the
> functions in the .c file whereas compilers would optimize away them if
> they are static inline functions.

The functions will be 1 or 2 bytes and can all be deduplicated, the
linker can also garbage collect them. It is better to optimize code
for readability rather than for wins like this.

Thanks,
Ian