On Thu, Sep 24, 2015 at 11:35 PM, Hemant Kumar
<hem...@linux.vnet.ibm.com> wrote:
>
>
> 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() ?
>
Yes, this is true.
Why are you asking?
> 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
