intel_pmu: add metadata with information about scaling #2398
Conversation
Change-Id: I259455d35aac9edef8f05310f199637f3ce09491 Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>
src/intel_pmu.c
Outdated
| @@ -283,6 +285,27 @@ static void pmu_submit_counter(int cpu, char *event, counter_t value) { | |||
| plugin_dispatch_values(&vl); | |||
| } | |||
|
|
|||
| meta_data_t *pmu_event_get_meta(struct event *e, int cpu) { | |||
There was a problem hiding this comment.
it would be great to rename this function to pmu_event_set_meta rather than get... but other than that this looks good to me
There was a problem hiding this comment.
Typically we call constructors (functions that allocate, initialize and return a foo_t) foo_create. The pmu prefix is fine, which would make this pmu_meta_data_create.
src/intel_pmu.c
Outdated
| pmu_submit_counter(i, e->event, value); | ||
| pmu_submit_counter(i, e->event, value, meta); | ||
|
|
||
| if (meta) { |
There was a problem hiding this comment.
the meta_data_destroy handles the null ptr, so the if statement is not required here.
There was a problem hiding this comment.
Just checking that you saw this comment: there is no need to check if meta is non-NULL and imho the code is cleaner without the check, but the decision is up to you.
(A good practice is to reply something along the lines of "ACK; I prefer to keep as-is" to optional comments to show that you considered them.)
There was a problem hiding this comment.
Ok
Wanted to keep as-is but if two reviewers say the code will be cleaner I agree :)
src/intel_pmu.c
Outdated
| @@ -283,6 +285,27 @@ static void pmu_submit_counter(int cpu, char *event, counter_t value) { | |||
| plugin_dispatch_values(&vl); | |||
| } | |||
|
|
|||
| meta_data_t *pmu_event_get_meta(struct event *e, int cpu) { | |||
There was a problem hiding this comment.
LGTM, thanks @serhiyx.
Thanks @maryamtahhan and @vmytnykx for your review comments, too :) Once the comments are addressed I'm happy to merge.
Best regards,
—octo
src/intel_pmu.c
Outdated
| @@ -283,6 +285,27 @@ static void pmu_submit_counter(int cpu, char *event, counter_t value) { | |||
| plugin_dispatch_values(&vl); | |||
| } | |||
|
|
|||
| meta_data_t *pmu_event_get_meta(struct event *e, int cpu) { | |||
There was a problem hiding this comment.
Typically we call constructors (functions that allocate, initialize and return a foo_t) foo_create. The pmu prefix is fine, which would make this pmu_meta_data_create.
src/intel_pmu.c
Outdated
| @@ -283,6 +285,27 @@ static void pmu_submit_counter(int cpu, char *event, counter_t value) { | |||
| plugin_dispatch_values(&vl); | |||
| } | |||
|
|
|||
| meta_data_t *pmu_event_get_meta(struct event *e, int cpu) { | |||
There was a problem hiding this comment.
You only ever seem to access e->efd[cpu] – can you pass that instead of e and cpu individually?
Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>
src/intel_pmu.c
Outdated
| meta_data_t *meta = NULL; | ||
|
|
||
| /* create meta data only if value was scaled */ | ||
| if (e->efd[cpu].val[1] != e->efd[cpu].val[2] && e->efd[cpu].val[2]) { | ||
| if (efd->val[1] != efd->val[2] && efd->val[2]) { |
There was a problem hiding this comment.
Please invert the logic to remove one level of indentation:
if (efd->val[1] == efd->val[2] || !efd->val[2]) {
return NULL;
}
meta_data_t *meta = meta_data_create();
/* … */
src/intel_pmu.c
Outdated
| pmu_submit_counter(i, e->event, value); | ||
| pmu_submit_counter(i, e->event, value, meta); | ||
|
|
||
| if (meta) { |
There was a problem hiding this comment.
Just checking that you saw this comment: there is no need to check if meta is non-NULL and imho the code is cleaner without the check, but the decision is up to you.
(A good practice is to reply something along the lines of "ACK; I prefer to keep as-is" to optional comments to show that you considered them.)
Signed-off-by: Serhiy Pshyk <serhiyx.pshyk@intel.com>
|
I manually triggered a build all is well: https://ci.collectd.org/job/pull-requests-manual_trigger-aggregation/248/ |
In case reported counters were scaled metadata contains information basing on which scaled value was calculated.
If there are more events than counters, the kernel uses time multiplexing. With multiplexing, at the end of the run, the counter is scaled basing on total time enabled vs time running: final_count = raw_count * time_enabled/time_running