On 09/24/2015 09:53 AM, Stephane Eranian wrote:
> Hi,
>
> Sorry for the delay.
No problem.
> Several remarks on this patch:
> - showevtinfo is a generic program that is meant to run even on non
> Linux systems, it cannot contain Linux specific knowledge
Thanks for explaining that. Will remove the showevtinfo changes.
> The rest is inlined below.
>
>
> On Mon, Sep 21, 2015 at 9:14 PM, 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,
>> unit}) for intel sandy-bridge uncore memory controllers' read and write
>> counters. These values are needed to scale the accumulated values to their
>> respective units.
>>
>> Signed-off-by: Hemant Kumar <hem...@linux.vnet.ibm.com>
>> ---
>> examples/showevtinfo.c | 5 ++-
>> include/perfmon/pfmlib.h | 8 ++++
>> lib/pfmlib_intel_x86.c | 103
>> +++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 115 insertions(+), 1 deletion(-)
>>
>> diff --git a/examples/showevtinfo.c b/examples/showevtinfo.c
>> index 45763ec..d182150 100644
>> --- a/examples/showevtinfo.c
>> +++ b/examples/showevtinfo.c
>> @@ -460,13 +460,16 @@ show_event_info(pfm_event_info_t *info)
>>
>> print_attr_flags(&ainfo);
>>
>> + printf(": %lf ", ainfo.scale);
>> + if (ainfo.unit == PFM_UNIT_MIB)
>> + printf(": MiB ");
>> +
> Why do you need to know it is MiB. The kernel exports everything you
> need. The perf tool has
> no knowledge of MiB. The unit is just a string which you can print.
Right. Will just export the string then.
>> putchar(':');
>>
>> if (ainfo.equiv)
>> printf(" Alias to %s", ainfo.equiv);
>> else
>> printf(" %s", ainfo.desc);
>> -
>> putchar('\n');
>> um++;
>> break;
>> diff --git a/include/perfmon/pfmlib.h b/include/perfmon/pfmlib.h
>> index 2fb4744..ceea202 100644
>> --- a/include/perfmon/pfmlib.h
>> +++ b/include/perfmon/pfmlib.h
>> @@ -428,6 +428,8 @@ typedef struct {
>> int dfl_bool; /* default boolean value */
>> int dfl_int; /* default integer value */
>> } SWIG_NAME(defaults);
>> + double scale;
>> + int unit;
>> } pfm_event_attr_info_t;
>>
>> /*
>> @@ -515,6 +517,12 @@ extern pfm_err_t pfm_get_event_encoding(const char
>> *str, int dfl_plm, char **fst
>> #define PFM_ERR_TOOMANY -11 /* too many parameters */
>> #define PFM_ERR_TOOSMALL -12 /* parameter is too small */
>>
>> +typedef enum {
>> + PFM_UNIT_UNKNOWN=0, /* unknown units type */
>> + PFM_UNIT_MIB, /* The unit is MiB */
>> + PFM_UNIT_MAX
>> +} pfm_attr_unit_t;
[SNIP]
>> +static int
>> +get_attr_unit(void *this, int pidx, int idx, intel_x86_umask_t umask)
>> +{
>> + char unit_path[PATH_MAX];
>> + FILE *fp;
>> + size_t len = 0;
>> + char *line = NULL, *unit_file = NULL;
>> + ssize_t ret;
>> + int unit;
>> +
>> + unit_file = get_attr_file_name(this, pidx, idx, umask);
>> + if (!unit_file)
>> + goto end;
>> +
>> + sprintf(unit_path, "/sys/bus/event_source/devices/%s/events/%s.unit",
>> + (((pfmlib_pmu_t *)this)->perf_name), unit_file);
>> + if (unit_file != NULL)
>> + free(unit_file);
>> +
>> + fp = fopen(unit_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';
>> + if (!strcmp(line, "MiB"))
>> + unit = PFM_UNIT_MIB;
>> + free(line);
>> + fclose(fp);
>> + return unit;
>> + }
>> +end:
>> + return PFM_UNIT_UNKNOWN;
>> +}
>> +
>> int
>> pfm_intel_x86_get_event_attr_info(void *this, int pidx, int attr_idx,
>> pfm_event_attr_info_t *info)
>> {
>> @@ -915,6 +1016,8 @@ pfm_intel_x86_get_event_attr_info(void *this, int pidx,
>> int attr_idx, pfm_event_
>> info->type = PFM_ATTR_UMASK;
>> info->is_dfl = intel_x86_uflag(this, pidx, idx,
>> INTEL_X86_DFL);
>> info->is_precise = intel_x86_uflag(this, pidx, idx,
>> INTEL_X86_PEBS);
>> + info->scale = get_attr_scale(this, pidx, idx,
>> pe[pidx].umasks[idx]);
>> + info->unit = get_attr_unit(this, pidx, idx,
>> pe[pidx].umasks[idx]);
> I think we could add those two fields to the info struct. However,
> that would need to come from the perf_event layer
> and not form the generic layer like you are doing now. In other words,
> those two info fields need to be filled out in an
> OS specific callback.
I have a question here. I may be wrong but Isn't
pfm_intel_x86_get_event_attr_info() being eventually called from
os->encode() through pfmlib_parse_event(),
pfmlib_build_event_pattrs() and pmu->get_event_attr_info() ?
Please let me know.
>> } else {
>> idx = intel_x86_attr2mod(this, pidx, attr_idx);
>> info->name = atdesc[idx].name;
>> --
>> 1.9.3
>>
Thanks for the review and the suggestions.
--
Hemant Kumar
------------------------------------------------------------------------------
_______________________________________________
perfmon2-devel mailing list
perfmon2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/perfmon2-devel
