[PATCH 5/5] ARM: perf: separate PMU backends into multiple files
Will Deacon
will.deacon at arm.com
Tue Nov 16 05:12:57 EST 2010
> Will,
>
> The checkpatch script returns some warnings and errors, cf. dump here below.
[...]
Cheers, I'll fix these for v2.
> Other than that I have a few remarks inlined.
[...]
> > -const struct arm_pmu *__init xscale2pmu_init(void)
> > -{
> > - return &xscale2pmu;
> > -}
> > +/* Include the PMU-specific implementations. */
> > +#include "perf_event_xscale.c"
> > +#include "perf_event_v6.c"
> > +#include "perf_event_v7.c"
> >
> > static int __init
> > init_hw_perf_events(void)
>
> It is better to use Kconfig/Makefile to conditionally compile files
> instead of using #include for C files.
As a general rule, I agree with you. In fact, I spent a large part of
Sunday afternoon trying to factor out these architectures so that the
`drivers' can be compiled individually and then interact with the main
perf code via an internal API. Whilst I eventually achieved this, the
code was *horrible* and massively over-engineered. The PMU backends
require access to a lot of internal types and structures which you
suddenly need to stick into a header file. The result is that perf_event.h
becomes full of ARM internal information and you end up with an elaborate
combination of forward declarations, #ifdefs and type information floating
about as a result of cleaning up the code!
At this point, I took a look at what x86 does and they use the #include
trick above. Whilst it's not code I would immediately think of writing, the
result is a much cleaner main file in my opinion. In fact, you could probably
drop the first 4 patches of this series and it would still work, but they
exist because of the initial approach I took and I still believe that making
the architecture files as self-contained as possible is a good thing.
For what it's worth, the perf_event_*.c files do contain internal compile-time
guards so they expand to a minimal (empty) init function if you're not compiling
for the relevant architecture.
Will
More information about the linux-arm-kernel
mailing list