From: Ingo Molnar <mingo@elte.hu>
To: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>,
Don Zickus <dzickus@redhat.com>,
LKML <linux-kernel@vger.kernel.org>,
Stephane Eranian <eranian@google.com>,
Lin Ming <ming.m.lin@intel.com>,
Arnaldo Carvalho de Melo <acme@redhat.com>,
Frederic Weisbecker <fweisbec@gmail.com>,
Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH -tip/perf/core] perf, x86: P4 PMU - Introduce event alias feature
Date: Thu, 21 Jul 2011 10:15:31 +0200 [thread overview]
Message-ID: <20110721081531.GI9216@elte.hu> (raw)
In-Reply-To: <20110721080755.GO7492@sun>
* Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Thu, Jul 21, 2011 at 09:52:01AM +0200, Ingo Molnar wrote:
> >
> > * Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> >
> > > > > +static u64 p4_get_alias_event(u64 config)
> > > > > +{
> > > > > + u64 config_match;
> > > > > + int i;
> > > > > +
> > > > > + /*
> > > > > + * Probably we're lucky and don't have to do
> > > > > + * matching over all config bits.
> > > > > + */
> > > > > + if (!(config & P4_CONFIG_ALIASABLE))
> > > > > + return 0;
> > > >
> > > > 'all' config bits? There's a single alias mapping in
> > > > p4_event_aliases[] right now which makes the comment rather
> > > > misleading ...
> > >
> > > no, the cycle below does check for all bits in config, and test
> > > for single bit might help us to return early.
> >
> > The loop below? AFAICS it does:
> >
> > > > > + */
> > > > > + for (i = 0; i < ARRAY_SIZE(p4_event_aliases); i++) {
> > > > > + if (config_match == p4_event_aliases[i].orig) {
> > > > > + config_match = p4_event_aliases[i].alter;
> > > > > + break;
> > > > > + } else if (config_match == p4_event_aliases[i].alter) {
> > > > > + config_match = p4_event_aliases[i].orig;
> > > > > + break;
> > > > > + }
> > > > > + }
> >
> > and p4_event_aliases[] is a single-entry array, so ARRAY_SIZE() is 1
> > and this loop iterates once.
>
> I know, but Ingo, I can't guarantee that we'll never bring some new
> alias(es) later ;) [...]
in which 'later' case that check and the comment can be added or
updated ...
Right now it looks both somewhat pointless *and* the comment is
somewhat misleading. You could fix the comment, pointing out that
right now it's only a single entry but it's still a
micro-optimization.
and if there's more entries in the future the comment can be updated.
> > > > Since this .c file is P4 specific and p4_event_aliases[] is a
> > > > file-scope array, is the p4_ prefix even needed?
> > >
> > > I prefer them to be distinguished this way, since the .c file in
> > > real is included into another (perf_event.c) file.
> >
> > That should probably be fixed btw. - but yeah, until the PMU drivers
> > are separated out better the prefix is fine.
>
> I thought about adding some additional Kconfig where we would
> choose which PMU to compile in, not sure though where is the best
> place for it (under which menu section I mean).
No need for any additional Kconfig for that, we could just build them
as separate .o's.
Thanks,
Ingo
next prev parent reply other threads:[~2011-07-21 8:16 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-07-08 20:17 [PATCH -tip/perf/core] perf, x86: P4 PMU - Introduce event alias feature Cyrill Gorcunov
2011-07-14 18:46 ` Don Zickus
2011-07-21 6:46 ` Ingo Molnar
2011-07-21 7:20 ` Cyrill Gorcunov
2011-07-21 7:52 ` Ingo Molnar
2011-07-21 8:07 ` Cyrill Gorcunov
2011-07-21 8:15 ` Ingo Molnar [this message]
2011-07-21 12:02 ` Peter Zijlstra
2011-07-21 10:00 ` [tip:perf/core] " tip-bot for Cyrill Gorcunov
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=20110721081531.GI9216@elte.hu \
--to=mingo@elte.hu \
--cc=a.p.zijlstra@chello.nl \
--cc=acme@redhat.com \
--cc=dzickus@redhat.com \
--cc=eranian@google.com \
--cc=fweisbec@gmail.com \
--cc=gorcunov@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=ming.m.lin@intel.com \
--cc=rostedt@goodmis.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).