Messages in this thread | | | Date | Thu, 21 Jul 2011 09:52:01 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH -tip/perf/core] perf, x86: P4 PMU - Introduce event alias feature |
| |
* 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.
> > 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.
> > > + > > > + if (i >= ARRAY_SIZE(p4_event_aliases)) > > > + return 0; > > > + > > > + return (config_match | > > > + (config & P4_CONFIG_EVENT_ALIAS_IMMUTABLE_BITS)); > > > > 'return' is not a function. Also, please don't break the line > > pointlessly. > > It is perfectly fine to return args in braces, > but I will change it.
The parantheses are syntactically valid but not 'perfectly fine'. As per kernel coding style we don't use them, to signal that 'return' is not a function.
> > ... > > > + /* > > > + * Probably an event alias is still available. > > > > s/Probably/Possibly? > > Hm? I don't get what is wrong with 'probably'.
Probably means "most likely, maybe" - and that's not the case here.
(It's misleading in this context because it declares the probability of this action as always higher than 50% - and that's not always the case here.)
'Possibly' leaves the probability more undefined.
You could also write 'Check whether an event alias is still available'.
Thanks,
Ingo
| |