Re: [PATCH 1/7] perf stat: Initialize instead of overwriting clock event

From: James Clark
Date: Tue Aug 13 2024 - 10:39:04 EST




On 13/08/2024 3:28 pm, Ian Rogers wrote:
On Tue, Aug 13, 2024 at 6:24 AM James Clark <james.clark@xxxxxxxxxx> wrote:

This overwrite relies on the clock event remaining at index 0 and is
quite a way down from where the array is initialized, making it easy to
miss. Just initialize it with the correct clock event to begin with.

Signed-off-by: James Clark <james.clark@xxxxxxxxxx>
---
tools/perf/builtin-stat.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 1f92445f7480..a65f58f8783f 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1817,7 +1817,9 @@ static int add_default_attributes(void)
{
struct perf_event_attr default_attrs0[] = {

- { .type = PERF_TYPE_SOFTWARE, .config = PERF_COUNT_SW_TASK_CLOCK },
+ { .type = PERF_TYPE_SOFTWARE, .config = target__has_cpu(&target) ?
+ PERF_COUNT_SW_CPU_CLOCK :
+ PERF_COUNT_SW_TASK_CLOCK },

Hand crafting perf_event_attr when we have an event name to
perf_event_atttr parser doesn't make sense. Doing things this way
means we need to duplicate logic between event parsing and these
default configurations. The default configurations are also using
legacy events which of course are broken on Apple ARM M? (albeit for
hardware events, here it is software). Event and metric parsing has to
worry about things like grouping topdown events. All-in-all let's have
one way to do things, event parsing, otherwise this code is going to
end up reinventing all the workarounds the event parsing has to have.
Lots of struct perf_event_attr also contribute to binary size.

If you are worried about a cycles event being opened on arm_dsu PMUs,
there is this patch:
https://lore.kernel.org/lkml/20240525152927.665498-1-irogers@xxxxxxxxxx/

Thanks,
Ian

Hi Ian,

Is this comment related to this patch specifically or is it more of a general comment?

This patch doesn't really make any actual changes other than move one line of code from one place to another.

James