lkml.org 
[lkml]   [2012]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/6] perf tool: Parse general events from sysfs
    On Wed, Apr 25, 2012 at 01:47:46PM +0800, Yan, Zheng wrote:
    > On 04/24/2012 05:04 PM, Jiri Olsa wrote:
    > > hi,
    > > so the point is to have an alias support for pmu event definition.
    > > Seems like good idea to improve usability/readability, I have some
    > > general comments though..
    > >
    > Thank you very much for reviewing this.
    > > - you suggest to have sysfs files having contents like:
    > > event=0x2c,umask=0xf
    > > when we were adding the formats stuff into sysfs, we had to cut off
    > > the sysfs file contents to bare minimum to obey the sysfs rule:
    > > single file = single value
    > > so you might have some troubles pushing that through.. not sure ;)
    > >
    > > - I haven't read the whole patchset, but seems like the "events"
    > > directory is now specific to a 'Intel uncore pmu'. If thats the
    > > case I think there should be generic way for each pmu to define
    > > this stuff.
    > >
    > > - as for the tools/pmu.c change I'd like to see more consistent
    > > way of parsing this, than via 'newcfg' variable.. but none
    > > is comming to me so far ;) I'll think about that..
    > >
    > For this point, how about making the parser re-entrantable like the patch
    > attached below does.

    yep, looks cleaner ;) some comments below

    >
    > Regards
    > Yan, Zheng
    >
    > ---
    > diff --git a/tools/perf/Makefile b/tools/perf/Makefile
    > index e98e14c..a8b47d2 100644
    > --- a/tools/perf/Makefile
    > +++ b/tools/perf/Makefile
    > @@ -728,11 +728,13 @@ $(OUTPUT)perf.o perf.spec \
    > # These two need to be here so that when O= is not used they take precedence

    SNIP

    > +static int __parse_events(const char *str, int *idx, struct list_head *list);
    >
    > #define CHW(x) .type = PERF_TYPE_HARDWARE, .config = PERF_COUNT_HW_##x
    > #define CSW(x) .type = PERF_TYPE_SOFTWARE, .config = PERF_COUNT_SW_##x
    > @@ -651,8 +653,11 @@ int parse_events_add_numeric(struct list_head *list, int *idx,
    > int parse_events_add_pmu(struct list_head *list, int *idx,
    > char *name, struct list_head *head_config)
    > {
    > + LIST_HEAD(event);

    Are there all available events under ...event_source/devices/%s/events or
    just some of them ?

    In case of the latter, maybe the 'aliases/alias' would be better name for
    that.. ending up with rename in this source ;)


    > struct perf_event_attr attr;
    > struct perf_pmu *pmu;
    > + char *config;
    > + int ret;
    >
    > pmu = perf_pmu__find(name);
    > if (!pmu)
    > @@ -666,10 +671,21 @@ int parse_events_add_pmu(struct list_head *list, int *idx,
    > */
    > config_attr(&attr, head_config, 0);
    >
    > - if (perf_pmu__config(pmu, &attr, head_config))
    > - return -EINVAL;
    > + ret = perf_pmu__config(pmu, &attr, head_config);
    > + if (!ret)
    > + return add_event(list, idx, &attr, (char *) "pmu");
    > +
    > + ret = perf_pmu__alias(pmu, &config, head_config);
    > + if (ret)
    > + return ret;


    I think it's ok for pmu object to contain list of aliases loaded
    from sysfs (if that change will go throught ;) ), but I'd rather
    see the pmu alias interface to be more generic like:

    config = perf_pmu__alias(pmu, alias);

    The function would just return the event config string if the record
    is found for the alias.

    ...and the parse_events_add_pmu function would then take care of
    the rest.. the final 'pmu/config' string and have it parsed...


    thanks,
    jirka


    \
     
     \ /
      Last update: 2012-04-25 15:05    [W:0.028 / U:0.120 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site