All of lore.kernel.org
 help / color / mirror / Atom feed
From: will.deacon@arm.com (Will Deacon)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 5/5] ARM: perf: separate PMU backends into multiple files
Date: Tue, 16 Nov 2010 10:12:57 -0000	[thread overview]
Message-ID: <006101cb8576$d7f6c520$87e44f60$@deacon@arm.com> (raw)
In-Reply-To: <AANLkTik6gEvTXh75_FouQegm_WxdSm8FmW2HB85jdW+T@mail.gmail.com>

> 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

  reply	other threads:[~2010-11-16 10:12 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-11-15 17:30 [PATCH 0/5] ARM: perf: split up perf_event.c by architecture Will Deacon
2010-11-15 17:30 ` [PATCH 1/5] ARM: perf: consolidate common PMU behaviour Will Deacon
2010-11-16  8:59   ` Jean Pihet
2010-11-16  9:47     ` Will Deacon
2010-11-16  9:16   ` Jamie Iles
2010-11-15 17:31 ` [PATCH 2/5] ARM: perf: avoid exposing internal stop function for v6 PMU Will Deacon
2010-11-15 19:02   ` Jamie Iles
2010-11-16  9:57     ` Will Deacon
2010-11-15 17:31 ` [PATCH 3/5] ARM: perf: add _init() functions to PMUs Will Deacon
2010-11-16  9:00   ` Jean Pihet
2010-11-16  9:18   ` Jamie Iles
2010-11-15 17:31 ` [PATCH 4/5] ARM: perf: encode PMU name in arm_pmu structure Will Deacon
2010-11-15 19:03   ` Jamie Iles
2010-11-16  8:29     ` Jean Pihet
2010-11-15 17:31 ` [PATCH 5/5] ARM: perf: separate PMU backends into multiple files Will Deacon
2010-11-16  9:11   ` Jean Pihet
2010-11-16 10:12     ` Will Deacon [this message]
2010-11-16  8:32 ` [PATCH 0/5] ARM: perf: split up perf_event.c by architecture Jean Pihet
2010-11-16  9:38   ` Will Deacon

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='006101cb8576$d7f6c520$87e44f60$@deacon@arm.com' \
    --to=will.deacon@arm.com \
    --cc=linux-arm-kernel@lists.infradead.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.