Messages in this thread | | | Date | Mon, 3 Jul 2023 14:09:05 +0100 | Subject | Re: [PATCH RFC 2/9] perf metrics: Don't iter sys metrics if we already found a CPU match | From | John Garry <> |
| |
On 30/06/2023 18:41, Ian Rogers wrote: > On Wed, Jun 28, 2023 at 3:30 AM John Garry<john.g.garry@oracle.com> wrote: >> In metricgroup__add_metric() we still iter the sys metrics if we already >> found a match from the CPU table, which is pretty pointless, so don't >> bother. >> >> Signed-off-by: John Garry<john.g.garry@oracle.com> >> --- >> tools/perf/util/metricgroup.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c >> index 4389ccd29fe7..8d2ac2513530 100644 >> --- a/tools/perf/util/metricgroup.c >> +++ b/tools/perf/util/metricgroup.c >> @@ -1261,6 +1261,12 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con >> >> has_match = data.has_match; >> }
Hi Ian,
>> + >> + if (has_match) { >> + ret = 0; >> + goto out; >> + } >> + > I think this can just be: > > if (!has_match)
But ret has no initial value
> > However, I'm not sure I agree with the intent of the change. We may > have a metric like IPC and want it to apply to all types of CPU, GPU, > etc. If we short-cut here then that won't be possible.
A few points to make on this: - Currently we don't have any same-named metrics like this, so not much use in supporting it in the code (yet). - Even if we had some same-named metrics, I am not sure if it even works properly. Do we have any uncore PMU metrics which have same name as CPU metrics? - Further to the previous point, do we really want same-named metrics for different PMUs in the future? I think event / metric names need to be chosen carefully to avoid clash for other PMUs or keywords. For your example, if I did ask for IPC metric, I'd like to be able to just know I'm getting IPC metric for CPUs or some other PMUs, but not both.
Thanks, John
>
| |