Re: [PATCH v4 2/4] perf stat: add event unit and scale support

[Stephane Eranian]

Jiri,


Thanks for the patch. I tested it and it works fine.
I will merge it with my "unit and scale" and add
your Signed-off-by. Is that okay with you?



On Fri, Nov 1, 2013 at 11:57 AM, Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> On Thu, Oct 31, 2013 at 03:59:40PM +0100, Stephane Eranian wrote:
>> This patch adds perf stat support fo rhandling event units and
>> scales as exported by the kernel.
>>
>> The kernel can export PMU events actual unit and scaling factor
>> via sysfs:
>> $ ls -1 /sys/devices/power/events/energy-*
>> /sys/devices/power/events/energy-cores
>> /sys/devices/power/events/energy-cores.scale
>> /sys/devices/power/events/energy-cores.unit
>> /sys/devices/power/events/energy-pkg
>> /sys/devices/power/events/energy-pkg.scale
>> /sys/devices/power/events/energy-pkg.unit
>> $ cat /sys/devices/power/events/energy-cores.scale
>> 2.3e-10
>> $ cat cat /sys/devices/power/events/energy-cores.unit
>> Joules
>>
>> This patch modifies the pmu event alias code to check
>> for the presence of the .unit and .scale files to load
>> the corresponding values. They are then used by perf stat
>> transparentely:
>>
>
> wrt our previous conversation, attached patch is what
> I meant (based on your changes)
>
> you could get rid of following functions:
>   pmu_event_unit
>   pmu_event_scale
>   pmu_get_event_unit_scale
>
> and have the unit/scale processing within the alias code
>
> the name that matters is the term name which is available
> in the parse_events_add_pmu as one of the terms
>
> jirka
> ---
>  tools/perf/util/parse-events.c | 29 ++++++++-----
>  tools/perf/util/pmu.c          | 92 +++++++++++++++---------------------------
>  tools/perf/util/pmu.h          |  6 +--
>  3 files changed, 53 insertions(+), 74 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index be7eba8..90ae328 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -269,9 +269,10 @@ const char *event_type(int type)
>
>
>
> -static int __add_event(struct list_head *list, int *idx,
> -                      struct perf_event_attr *attr,
> -                      char *name, struct cpu_map *cpus)
> +static struct perf_evsel *
> +__add_event(struct list_head *list, int *idx,
> +           struct perf_event_attr *attr,
> +           char *name, struct cpu_map *cpus)
>  {
>         struct perf_evsel *evsel;
>
> @@ -279,19 +280,19 @@ static int __add_event(struct list_head *list, int *idx,
>
>         evsel = perf_evsel__new(attr, (*idx)++);
>         if (!evsel)
> -               return -ENOMEM;
> +               return NULL;
>
>         evsel->cpus = cpus;
>         if (name)
>                 evsel->name = strdup(name);
>         list_add_tail(&evsel->node, list);
> -       return 0;
> +       return evsel;
>  }
>
>  static int add_event(struct list_head *list, int *idx,
>                      struct perf_event_attr *attr, char *name)
>  {
> -       return __add_event(list, idx, attr, name, NULL);
> +       return __add_event(list, idx, attr, name, NULL) ? 0 : -ENOMEM;
>  }
>
>  static int parse_aliases(char *str, const char *names[][PERF_EVSEL__MAX_ALIASES], int size)
> @@ -633,6 +634,9 @@ int parse_events_add_pmu(struct list_head *list, int *idx,
>  {
>         struct perf_event_attr attr;
>         struct perf_pmu *pmu;
> +       struct perf_evsel *evsel;
> +       char *unit;
> +       double scale;
>
>         pmu = perf_pmu__find(name);
>         if (!pmu)
> @@ -640,7 +644,7 @@ int parse_events_add_pmu(struct list_head *list, int *idx,
>
>         memset(&attr, 0, sizeof(attr));
>
> -       if (perf_pmu__check_alias(pmu, head_config))
> +       if (perf_pmu__check_alias(pmu, head_config, &unit, &scale))
>                 return -EINVAL;
>
>         /*
> @@ -652,8 +656,14 @@ int parse_events_add_pmu(struct list_head *list, int *idx,
>         if (perf_pmu__config(pmu, &attr, head_config))
>                 return -EINVAL;
>
> -       return __add_event(list, idx, &attr, pmu_event_name(head_config),
> -                          pmu->cpus);
> +       evsel = __add_event(list, idx, &attr, pmu_event_name(head_config),
> +                           pmu->cpus);
> +       if (evsel) {
> +               evsel->unit = unit;
> +               evsel->scale = scale;
> +       }
> +
> +       return evsel ? 0 : -ENOMEM;
>  }
>
>  int parse_events__modifier_group(struct list_head *list,
> @@ -838,7 +848,6 @@ int parse_events_name(struct list_head *list, char *name)
>         list_for_each_entry(evsel, list, node) {
>                 if (!evsel->name)
>                         evsel->name = strdup(name);
> -               pmu_get_event_unit_scale(evsel);
>         }
>
>         return 0;
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index e18ef87..df68d43 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -620,16 +620,42 @@ static struct perf_pmu_alias *pmu_find_alias(struct perf_pmu *pmu,
>         return NULL;
>  }
>
> +
> +static int check_unit_scale(struct perf_pmu_alias *alias,
> +                           char **unit, double *scale)
> +{
> +       /*
> +        * Only one term in event definition can
> +        * define unit and scale, fail if there's
> +        * more than one.
> +        */
> +       if ((*unit && alias->unit) ||
> +           (*scale && alias->scale))
> +               return -EINVAL;
> +
> +       if (alias->unit)
> +               *unit = alias->unit;
> +
> +       if (alias->scale)
> +               *scale = alias->scale;
> +
> +       return 0;
> +}
> +
>  /*
>   * Find alias in the terms list and replace it with the terms
>   * defined for the alias
>   */
> -int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms)
> +int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
> +                         char **unit, double *scale)
>  {
>         struct parse_events_term *term, *h;
>         struct perf_pmu_alias *alias;
>         int ret;
>
> +       *unit   = NULL;
> +       *scale  = 0;
> +
>         list_for_each_entry_safe(term, h, head_terms, list) {
>                 alias = pmu_find_alias(pmu, term);
>                 if (!alias)
> @@ -637,6 +663,11 @@ int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms)
>                 ret = pmu_alias_terms(alias, &term->list);
>                 if (ret)
>                         return ret;
> +
> +               ret = check_unit_scale(alias, unit, scale);
> +               if (ret)
> +                       return ret;
> +
>                 list_del(&term->list);
>                 free(term);
>         }
> @@ -760,62 +791,3 @@ bool pmu_have_event(const char *pname, const char *name)
>         }
>         return false;
>  }
> -
> -static const char *pmu_event_unit(struct perf_pmu *pmu, const char *name)
> -{
> -       struct perf_pmu_alias *alias;
> -       char buf[1024];
> -       char *fname;
> -       const char *unit = "?";
> -
> -       if (!name)
> -               return unit;
> -
> -       list_for_each_entry(alias, &pmu->aliases, list) {
> -               fname = format_alias(buf, sizeof(buf), pmu, alias);
> -               if (!strcmp(fname, name)) {
> -                       unit = alias->unit;
> -                       break;
> -               }
> -       }
> -       return unit;
> -}
> -
> -static double pmu_event_scale(struct perf_pmu *pmu, const char *name)
> -{
> -       struct perf_pmu_alias *alias;
> -       char buf[1024];
> -       char *fname;
> -       double scale = 1.0;
> -
> -       if (!name)
> -               return 1.0;
> -
> -       list_for_each_entry(alias, &pmu->aliases, list) {
> -               fname = format_alias(buf, sizeof(buf), pmu, alias);
> -               if (!strcmp(fname, name)) {
> -                       scale = alias->scale;
> -                       break;
> -               }
> -       }
> -       return scale;
> -}
> -
> -int pmu_get_event_unit_scale(struct perf_evsel *evsel)
> -{
> -       __u32 type = evsel->attr.type;
> -       struct perf_pmu *pmu;
> -
> -       if (!evsel->name)
> -               return -1;
> -
> -       list_for_each_entry(pmu, &pmus, list) {
> -               if (pmu->type == type)
> -                       goto found;
> -       }
> -       return -1;
> -found:
> -       evsel->unit  = pmu_event_unit(pmu, evsel->name);
> -       evsel->scale = pmu_event_scale(pmu, evsel->name);
> -       return 0;
> -}
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 6bf23b2..9183380 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -4,7 +4,6 @@
>  #include <linux/bitops.h>
>  #include <linux/perf_event.h>
>  #include <stdbool.h>
> -#include "util/evsel.h"
>
>  enum {
>         PERF_PMU_FORMAT_VALUE_CONFIG,
> @@ -29,7 +28,8 @@ int perf_pmu__config(struct perf_pmu *pmu, struct perf_event_attr *attr,
>  int perf_pmu__config_terms(struct list_head *formats,
>                            struct perf_event_attr *attr,
>                            struct list_head *head_terms);
> -int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms);
> +int perf_pmu__check_alias(struct perf_pmu *pmu, struct list_head *head_terms,
> +                         char **unit, double *scale);
>  struct list_head *perf_pmu__alias(struct perf_pmu *pmu,
>                                   struct list_head *head_terms);
>  int perf_pmu_wrap(void);
> @@ -46,6 +46,4 @@ void print_pmu_events(const char *event_glob, bool name_only);
>  bool pmu_have_event(const char *pname, const char *name);
>
>  int perf_pmu__test(void);
> -
> -int pmu_get_event_unit_scale(struct perf_evsel *evsel);
>  #endif /* __PMU_H */
> --
> 1.7.11.7
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/