Re: [PATCH v6 15/22] perf lock: Move common lock contention code to new file

[Namhyung Kim <namhyung@kernel.org>]

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@kernel.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.

Thanks,
Namhyung