Hi,
There are several things wrong with this patch.
My understanding is that the goal it to be able to retrieve the scaling
factor exported by the kernel
for certain events.
First of all, scaling is only exported for kernel exported events. It is
not for any generic event.
The way this works in sysfs is by matching the event name, with the .scale
or .unit files.
For instance on my laptop:
$ ls -1 /sys/devices/uncore_imc/events/
data_reads
data_reads.scale
data_reads.unit
data_writes
data_writes.scale
data_writes.unit
Unless libpfm4 event name is identical to the name used in sysfs (here,
data_reads for instance), you are
not going to be able to extract a scaling factor. Libpfm4 may call the
event differently. To be more robust,
you'd have to use the event encoding and look for a match in the
/sys/devices/XXX/events/* and then
compare the config=X value with what you have.
So I am trying to gauge the usefulness of this new feature.
The patch needs some serious revision in any case. See my comments below.
On Tue, Feb 9, 2016 at 12:17 AM, Hemant Kumar <hem...@linux.vnet.ibm.com>
wrote:
> Read the scale and unit values from the
> sysfs
> (/sys/bus/event_source/devices/uncore_imc_*/events/cas_count_{read,write}.scale)
> for intel sandy-bridge uncore memory controllers' read and write
> counters. These values are needed to scale the counter values.
>
> This patch also adds a function pfm_get_os_event_scale() to the libpfm
> API so that tools using perfmon/libpfm4 library can get the value of the
> scale similar to how pfm_get_os_event_encoding() gets the event
> encoding.
>
> Signed-off-by: Hemant Kumar <hem...@linux.vnet.ibm.com>
> ---
> include/perfmon/pfmlib.h | 6 ++++
> lib/pfmlib_common.c | 31 ++++++++++++++++++++
> lib/pfmlib_intel_x86.c | 25 ++++++++++++++++
> lib/pfmlib_perf_event.c | 75
> ++++++++++++++++++++++++++++++++++++++++++++++++
> lib/pfmlib_priv.h | 1 +
> 5 files changed, 138 insertions(+)
>
> diff --git a/include/perfmon/pfmlib.h b/include/perfmon/pfmlib.h
> index 2fb4744..543a8b2 100644
> --- a/include/perfmon/pfmlib.h
> +++ b/include/perfmon/pfmlib.h
> @@ -412,6 +412,7 @@ typedef struct {
> const char *name; /* attribute symbolic name */
> const char *desc; /* attribute description */
> const char *equiv; /* attribute is equivalent to */
> + const char *uname;
>
Describe what the field is about like for the other fields
> size_t size; /* struct sizeof */
> uint64_t code; /* attribute code */
> pfm_attr_t type; /* attribute type */
> @@ -484,6 +485,11 @@ extern pfm_err_t pfm_get_event_info(int idx, pfm_os_t
> os, pfm_event_info_t *outp
> extern pfm_err_t pfm_get_os_event_encoding(const char *str, int dfl_plm,
> pfm_os_t os, void *args);
>
> /*
> + * event scale API
> + */
> +extern pfm_err_t pfm_get_os_event_scale(const char *str, int dfl_plm,
> pfm_os_t os, void *args);
> +
>
Not clear why you need the priv level mask to determine the scaling factor.
> +/*
> * attribute API
> */
> extern pfm_err_t pfm_get_event_attr_info(int eidx, int aidx, pfm_os_t os,
> pfm_event_attr_info_t *output);
> diff --git a/lib/pfmlib_common.c b/lib/pfmlib_common.c
> index a6a364e..a108ada 100644
> --- a/lib/pfmlib_common.c
> +++ b/lib/pfmlib_common.c
> @@ -1350,10 +1350,34 @@ pfm_get_event_next(int idx)
> }
>
> int
> +pfm_get_os_event_scale(const char *str, int dfl_plm, pfm_os_t uos, void
> *args)
> +{
> + pfmlib_os_t *os;
> +
> + if (PFMLIB_INITIALIZED() == 0)
> + return PFM_ERR_NOINIT;
> +
> + if (dfl_plm & ~(PFM_PLM_ALL))
> + return PFM_ERR_INVAL;
> +
> + os = pfmlib_find_os(uos);
> + if (!os)
> + return PFM_ERR_NOTSUPP;
> +
> + return os->get_scale(os, str, dfl_plm, args);
> +}
> +
> +int
> pfm_get_os_event_encoding(const char *str, int dfl_plm, pfm_os_t uos,
> void *args)
> {
> pfmlib_os_t *os;
>
> + FILE *fp;
> +
> + fp = fopen("/home/hemant/tmp.txt", "w");
> + if (!fp)
> + return PFM_ERR_NOINIT;
> + fprintf(fp, "os->name : %s\n", os->name);
>
I am assuming this is leftover debugging code.
> if (PFMLIB_INITIALIZED() == 0)
> return PFM_ERR_NOINIT;
>
> @@ -1981,10 +2005,17 @@ pfmlib_raw_pmu_detect(void *this)
> return PFM_SUCCESS;
> }
>
> +static int
> +pfmlib_raw_pmu_get_scale(void *this, const char *str, int dfl_plm, void
> *data)
> +{
> + return PFM_ERR_NOTSUPP;
> +}
> +
> static pfmlib_os_t pfmlib_os_none= {
> .name = "No OS (raw PMU)",
> .id = PFM_OS_NONE,
> .flags = PFMLIB_OS_FL_ACTIVATED,
> .encode = pfmlib_raw_pmu_encode,
> .detect = pfmlib_raw_pmu_detect,
> + .get_scale = pfmlib_raw_pmu_get_scale,
> };
> diff --git a/lib/pfmlib_intel_x86.c b/lib/pfmlib_intel_x86.c
> index 81a2258..cb76cd1 100644
> --- a/lib/pfmlib_intel_x86.c
> +++ b/lib/pfmlib_intel_x86.c
> @@ -894,6 +894,30 @@ skip_dfl:
> return error ? PFM_ERR_INVAL : PFM_SUCCESS;
> }
>
> +#define MAX_LEN 100
> +
> +static char *
> +get_attr_uname(void *this, int pidx, int idx)
> +{
> + const intel_x86_entry_t *pe = this_pe(this);
> + char *text = NULL;
> +
> + if (!strcmp(pe[pidx].name, "UNC_M_CAS_COUNT")) {
> + text = calloc(MAX_LEN, sizeof(char));
> + if (!text)
> + return NULL;
> + strcpy(text, "cas_count_");
> + }
> + if (text) {
> + if (!strcmp(pe[pidx].umasks[idx].uname, "RD"))
> + strcat(text, "read");
> + else if(!strcmp(pe[pidx].umasks[idx].uname, "WR"))
> + strcat(text, "write");
> + }
> +
> + return text;
> +}
> +
>
I don't understand what this function is about and why it hardcodes IMC
event names
and who knows for which PMU model.
But I am guessing this is about substituting the libpfm4 event name with
the kernel name so you can
find the scaling file. This is the problem I was alluding to earlier. I
think this cannot be solved this way.
The kernel equivalent name needs to be encoded in the event table in as a
separate table in the
pfmlib_XXX.c file. Your code does not scale well when you have more events
for which you have
to swap names.
> int
> pfm_intel_x86_get_event_attr_info(void *this, int pidx, int attr_idx,
> pfm_event_attr_info_t *info)
> {
> @@ -906,6 +930,7 @@ pfm_intel_x86_get_event_attr_info(void *this, int
> pidx, int attr_idx, pfm_event_
> idx = intel_x86_attr2umask(this, pidx, attr_idx);
> info->name = pe[pidx].umasks[idx].uname;
> info->desc = pe[pidx].umasks[idx].udesc;
> + info->uname = get_attr_uname(this, pidx, idx);
> info->equiv= pe[pidx].umasks[idx].uequiv;
>
> info->code = pe[pidx].umasks[idx].ucode;
> diff --git a/lib/pfmlib_perf_event.c b/lib/pfmlib_perf_event.c
> index 8618d60..7f191f0 100644
> --- a/lib/pfmlib_perf_event.c
> +++ b/lib/pfmlib_perf_event.c
> @@ -73,6 +73,79 @@ static const pfmlib_attr_desc_t perf_event_ext_mods[]={
> PFM_ATTR_NULL /* end-marker to avoid exporting number of entries */
> };
>
> +static double
> +get_perf_scale(const char *pmu, const char *event)
> +{
> + char scale_path[PATH_MAX];
> +
> + FILE *fp;
> + size_t len = 0;
> + char *line = NULL;
> + ssize_t ret;
> + double scale;
> +
> + sprintf(scale_path,
> "/sys/bus/event_source/devices/%s/events/%s.scale",
> + pmu, event);
> +
> + fp = fopen(scale_path, "r");
> + if (!fp)
> + goto end;
> +
> + ret = getline(&line, &len, fp);
> + if (ret > 0) {
> + /* Remove the new line from the end of the string here */
> + if (line[strlen(line) - 1] == '\n')
> + line[strlen(line) - 1] = '\0';
> +
> + scale = atof(line);
> + free(line);
> + fclose(fp);
> + return scale;
> + }
> + if (fp)
>
You don't need the test here given you skip to end: in case of error
+ fclose(fp);
+end:
> + return 1.0;
> +}
> +
> +static int
> +pfmlib_perf_event_scale(void *this, const char *str, int dfl_plm, void
> *data)
> +{
>
Why do you pass a void * when you could pass a double *?
> + pfmlib_os_t *os = this;
> + pfmlib_pmu_t *pmu;
> + pfmlib_event_desc_t e;
> + pfm_event_attr_info_t ainfo;
> + int ret;
> + double *scale = data;
> +
> + memset(&e, 0, sizeof(e));
> +
> + e.osid = os->id;
> + e.dfl_plm = dfl_plm;
> +
> + /* after this call, need to call pfmlib_release_event() */
> + ret = pfmlib_parse_event(str, &e);
> + if (ret != PFM_SUCCESS)
> + return ret;
> +
> + pmu = e.pmu;
> +
> + fprintf(stderr, "event name : %s\n", str);
> + fprintf(stderr, "PMU name : %s\n", (e.pmu)->perf_name);
> + fprintf(stderr, "attribute name : %d\n", (e.attrs)->id);
> +
>
that's debugging code. Should use DPRINTF() instead.
> + /* need to free up ainfo.uname */
> + ret = pmu->get_event_attr_info(pmu, e.event, (e.attrs)->id,
> &ainfo);
> + if (ainfo.uname) {
> + fprintf(stderr, "UNAME : %s\n", ainfo.uname);
> + *scale = get_perf_scale((e.pmu)->perf_name, ainfo.uname);
> + fprintf(stderr, "scale : %f\n", *scale);
>
debug code.
> + }
> +
> + pfmlib_release_event(&e);
> + return ret;
> +}
> +
> static int
> pfmlib_perf_event_encode(void *this, const char *str, int dfl_plm, void
> *data)
> {
> @@ -432,6 +505,7 @@ pfmlib_os_t pfmlib_os_perf={
> .get_os_attr_info = perf_get_os_attr_info,
> .get_os_nattrs = perf_get_os_nattrs,
> .encode = pfmlib_perf_event_encode,
> + .get_scale = pfmlib_perf_event_scale,
> };
>
> pfmlib_os_t pfmlib_os_perf_ext={
> @@ -442,6 +516,7 @@ pfmlib_os_t pfmlib_os_perf_ext={
> .get_os_attr_info = perf_get_os_attr_info,
> .get_os_nattrs = perf_get_os_nattrs,
> .encode = pfmlib_perf_event_encode,
> + .get_scale = pfmlib_perf_event_scale,
> };
>
>
> diff --git a/lib/pfmlib_priv.h b/lib/pfmlib_priv.h
> index b5e2d0c..cc6307a 100644
> --- a/lib/pfmlib_priv.h
> +++ b/lib/pfmlib_priv.h
> @@ -152,6 +152,7 @@ typedef struct {
> int (*get_os_attr_info)(void *this,
> pfmlib_event_desc_t *e);
> int (*get_os_nattrs)(void *this,
> pfmlib_event_desc_t *e);
> int (*encode)(void *this, const char
> *str, int dfl_plm, void *args);
> + int (*get_scale)(void *this, const
> char *str, int dfl_plm, void *args);
> } pfmlib_os_t;
>
> #define PFMLIB_OS_FL_ACTIVATED 0x1 /* OS layer detected */
> --
> 1.9.3
>
>
>
> ------------------------------------------------------------------------------
> Site24x7 APM Insight: Get Deep Visibility into Application Performance
> APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
> Monitor end-to-end web transactions and take corrective actions now
> Troubleshoot faster and improve end-user experience. Signup Now!
> http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
> _______________________________________________
> perfmon2-devel mailing list
> perfmon2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
perfmon2-devel mailing list
perfmon2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
