* [PATCH 1/2] perf x86/topdown: Make topdown metrics comparators be symmetric
@ 2024-10-11 11:02 Dapeng Mi
2024-10-11 11:02 ` [PATCH 2/2] perf x86/topdown: Refine helper arch_is_topdown_metrics() Dapeng Mi
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Dapeng Mi @ 2024-10-11 11:02 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Ian Rogers, Adrian Hunter, Alexander Shishkin,
Kan Liang
Cc: linux-perf-users, linux-kernel, Dapeng Mi, Dapeng Mi
The commit "3b5edc0421e2 (perf x86/topdown: Don't move topdown metric
events in group)" modifies topdown metrics comparator to move topdown
metrics events which are not in same group with previous event. But it
just modifies the 2nd comparator and causes the comparators become
asymmetric.
Thus modify the 1st topdown metrics comparator and make the two
comparators be symmetric, and refine the comments as well.
Suggested-by: Ian Rogers <irogers@google.com>
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
tools/perf/arch/x86/util/evlist.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
index 438e4639fa89..447a734e591c 100644
--- a/tools/perf/arch/x86/util/evlist.c
+++ b/tools/perf/arch/x86/util/evlist.c
@@ -52,7 +52,7 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
* 3,015,058 cycles
* <not supported> topdown-retiring
*
- * if slots event and topdown metrics events are in two groups, the group which
+ * If slots event and topdown metrics events are in two groups, the group which
* has topdown metrics events must contain only the topdown metrics event,
* otherwise topdown metrics event can't be regrouped correctly as well, e.g.
*
@@ -69,13 +69,16 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
return -1;
if (arch_is_topdown_slots(rhs))
return 1;
- /* Followed by topdown events. */
- if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
- return -1;
+
/*
- * Move topdown events forward only when topdown events
- * are not in same group with previous event.
+ * Move topdown metrics events forward only when topdown metrics
+ * events are not in same group with previous slots event. If
+ * topdown metrics events are already in same group with slots
+ * event, do nothing.
*/
+ if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
+ lhs->core.leader != rhs->core.leader)
+ return -1;
if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
lhs->core.leader != rhs->core.leader)
return 1;
base-commit: 57fb40d40f9543213ffadb04d07cfb2ba46edc26
--
2.40.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] perf x86/topdown: Refine helper arch_is_topdown_metrics()
2024-10-11 11:02 [PATCH 1/2] perf x86/topdown: Make topdown metrics comparators be symmetric Dapeng Mi
@ 2024-10-11 11:02 ` Dapeng Mi
2024-10-16 17:37 ` Namhyung Kim
2024-10-16 17:56 ` [PATCH 1/2] perf x86/topdown: Make topdown metrics comparators be symmetric Ian Rogers
2024-10-17 16:39 ` Namhyung Kim
2 siblings, 1 reply; 6+ messages in thread
From: Dapeng Mi @ 2024-10-11 11:02 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Ian Rogers, Adrian Hunter, Alexander Shishkin,
Kan Liang
Cc: linux-perf-users, linux-kernel, Dapeng Mi, Dapeng Mi
Leverage the existed function perf_pmu__name_from_config() to check if
an event is topdown metrics event. perf_pmu__name_from_config() goes
through the defined formats and figures out the config of pre-defined
topdown events.
This avoids to figure out the config of topdown pre-defined events with
hard-coded format strings "event=" and "umask=" and provides more
flexibility.
Suggested-by: Ian Rogers <irogers@google.com>
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
tools/perf/arch/x86/util/topdown.c | 39 +++++++-----------------------
1 file changed, 9 insertions(+), 30 deletions(-)
diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
index cb2c64928bc4..f63747d0abdf 100644
--- a/tools/perf/arch/x86/util/topdown.c
+++ b/tools/perf/arch/x86/util/topdown.c
@@ -41,43 +41,22 @@ bool arch_is_topdown_slots(const struct evsel *evsel)
return false;
}
-static int compare_topdown_event(void *vstate, struct pmu_event_info *info)
-{
- int *config = vstate;
- int event = 0;
- int umask = 0;
- char *str;
-
- if (!strcasestr(info->name, "topdown"))
- return 0;
-
- str = strcasestr(info->str, "event=");
- if (str)
- sscanf(str, "event=%x", &event);
-
- str = strcasestr(info->str, "umask=");
- if (str)
- sscanf(str, "umask=%x", &umask);
-
- if (event == 0 && *config == (event | umask << 8))
- return 1;
-
- return 0;
-}
-
bool arch_is_topdown_metrics(const struct evsel *evsel)
{
- struct perf_pmu *pmu = evsel__find_pmu(evsel);
int config = evsel->core.attr.config;
+ const char *name_from_config;
+ struct perf_pmu *pmu;
- if (!pmu || !pmu->is_core)
+ /* All topdown events have an event code of 0. */
+ if ((config & 0xFF) != 0)
return false;
- if (perf_pmu__for_each_event(pmu, false, &config,
- compare_topdown_event))
- return true;
+ pmu = evsel__find_pmu(evsel);
+ if (!pmu || !pmu->is_core)
+ return false;
- return false;
+ name_from_config = perf_pmu__name_from_config(pmu, config);
+ return name_from_config && strcasestr(name_from_config, "topdown");
}
/*
--
2.40.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] perf x86/topdown: Refine helper arch_is_topdown_metrics()
2024-10-11 11:02 ` [PATCH 2/2] perf x86/topdown: Refine helper arch_is_topdown_metrics() Dapeng Mi
@ 2024-10-16 17:37 ` Namhyung Kim
2024-10-16 17:55 ` Ian Rogers
0 siblings, 1 reply; 6+ messages in thread
From: Namhyung Kim @ 2024-10-16 17:37 UTC (permalink / raw)
To: Dapeng Mi, Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Adrian Hunter, Alexander Shishkin, Kan Liang, linux-perf-users,
linux-kernel, Dapeng Mi
Hi Ian,
On Fri, Oct 11, 2024 at 11:02:07AM +0000, Dapeng Mi wrote:
> Leverage the existed function perf_pmu__name_from_config() to check if
> an event is topdown metrics event. perf_pmu__name_from_config() goes
> through the defined formats and figures out the config of pre-defined
> topdown events.
>
> This avoids to figure out the config of topdown pre-defined events with
> hard-coded format strings "event=" and "umask=" and provides more
> flexibility.
>
> Suggested-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
Are you ok with this now?
Thanks,
Namhyung
> ---
> tools/perf/arch/x86/util/topdown.c | 39 +++++++-----------------------
> 1 file changed, 9 insertions(+), 30 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> index cb2c64928bc4..f63747d0abdf 100644
> --- a/tools/perf/arch/x86/util/topdown.c
> +++ b/tools/perf/arch/x86/util/topdown.c
> @@ -41,43 +41,22 @@ bool arch_is_topdown_slots(const struct evsel *evsel)
> return false;
> }
>
> -static int compare_topdown_event(void *vstate, struct pmu_event_info *info)
> -{
> - int *config = vstate;
> - int event = 0;
> - int umask = 0;
> - char *str;
> -
> - if (!strcasestr(info->name, "topdown"))
> - return 0;
> -
> - str = strcasestr(info->str, "event=");
> - if (str)
> - sscanf(str, "event=%x", &event);
> -
> - str = strcasestr(info->str, "umask=");
> - if (str)
> - sscanf(str, "umask=%x", &umask);
> -
> - if (event == 0 && *config == (event | umask << 8))
> - return 1;
> -
> - return 0;
> -}
> -
> bool arch_is_topdown_metrics(const struct evsel *evsel)
> {
> - struct perf_pmu *pmu = evsel__find_pmu(evsel);
> int config = evsel->core.attr.config;
> + const char *name_from_config;
> + struct perf_pmu *pmu;
>
> - if (!pmu || !pmu->is_core)
> + /* All topdown events have an event code of 0. */
> + if ((config & 0xFF) != 0)
> return false;
>
> - if (perf_pmu__for_each_event(pmu, false, &config,
> - compare_topdown_event))
> - return true;
> + pmu = evsel__find_pmu(evsel);
> + if (!pmu || !pmu->is_core)
> + return false;
>
> - return false;
> + name_from_config = perf_pmu__name_from_config(pmu, config);
> + return name_from_config && strcasestr(name_from_config, "topdown");
> }
>
> /*
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] perf x86/topdown: Refine helper arch_is_topdown_metrics()
2024-10-16 17:37 ` Namhyung Kim
@ 2024-10-16 17:55 ` Ian Rogers
0 siblings, 0 replies; 6+ messages in thread
From: Ian Rogers @ 2024-10-16 17:55 UTC (permalink / raw)
To: Namhyung Kim
Cc: Dapeng Mi, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Adrian Hunter, Alexander Shishkin, Kan Liang, linux-perf-users,
linux-kernel, Dapeng Mi
On Wed, Oct 16, 2024 at 10:37 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Ian,
>
> On Fri, Oct 11, 2024 at 11:02:07AM +0000, Dapeng Mi wrote:
> > Leverage the existed function perf_pmu__name_from_config() to check if
> > an event is topdown metrics event. perf_pmu__name_from_config() goes
> > through the defined formats and figures out the config of pre-defined
> > topdown events.
> >
> > This avoids to figure out the config of topdown pre-defined events with
> > hard-coded format strings "event=" and "umask=" and provides more
> > flexibility.
> >
> > Suggested-by: Ian Rogers <irogers@google.com>
> > Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> Are you ok with this now?
Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Ian
> Thanks,
> Namhyung
>
> > ---
> > tools/perf/arch/x86/util/topdown.c | 39 +++++++-----------------------
> > 1 file changed, 9 insertions(+), 30 deletions(-)
> >
> > diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> > index cb2c64928bc4..f63747d0abdf 100644
> > --- a/tools/perf/arch/x86/util/topdown.c
> > +++ b/tools/perf/arch/x86/util/topdown.c
> > @@ -41,43 +41,22 @@ bool arch_is_topdown_slots(const struct evsel *evsel)
> > return false;
> > }
> >
> > -static int compare_topdown_event(void *vstate, struct pmu_event_info *info)
> > -{
> > - int *config = vstate;
> > - int event = 0;
> > - int umask = 0;
> > - char *str;
> > -
> > - if (!strcasestr(info->name, "topdown"))
> > - return 0;
> > -
> > - str = strcasestr(info->str, "event=");
> > - if (str)
> > - sscanf(str, "event=%x", &event);
> > -
> > - str = strcasestr(info->str, "umask=");
> > - if (str)
> > - sscanf(str, "umask=%x", &umask);
> > -
> > - if (event == 0 && *config == (event | umask << 8))
> > - return 1;
> > -
> > - return 0;
> > -}
> > -
> > bool arch_is_topdown_metrics(const struct evsel *evsel)
> > {
> > - struct perf_pmu *pmu = evsel__find_pmu(evsel);
> > int config = evsel->core.attr.config;
> > + const char *name_from_config;
> > + struct perf_pmu *pmu;
> >
> > - if (!pmu || !pmu->is_core)
> > + /* All topdown events have an event code of 0. */
> > + if ((config & 0xFF) != 0)
> > return false;
> >
> > - if (perf_pmu__for_each_event(pmu, false, &config,
> > - compare_topdown_event))
> > - return true;
> > + pmu = evsel__find_pmu(evsel);
> > + if (!pmu || !pmu->is_core)
> > + return false;
> >
> > - return false;
> > + name_from_config = perf_pmu__name_from_config(pmu, config);
> > + return name_from_config && strcasestr(name_from_config, "topdown");
> > }
> >
> > /*
> > --
> > 2.40.1
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] perf x86/topdown: Make topdown metrics comparators be symmetric
2024-10-11 11:02 [PATCH 1/2] perf x86/topdown: Make topdown metrics comparators be symmetric Dapeng Mi
2024-10-11 11:02 ` [PATCH 2/2] perf x86/topdown: Refine helper arch_is_topdown_metrics() Dapeng Mi
@ 2024-10-16 17:56 ` Ian Rogers
2024-10-17 16:39 ` Namhyung Kim
2 siblings, 0 replies; 6+ messages in thread
From: Ian Rogers @ 2024-10-16 17:56 UTC (permalink / raw)
To: Dapeng Mi
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Adrian Hunter, Alexander Shishkin, Kan Liang,
linux-perf-users, linux-kernel, Dapeng Mi
On Fri, Oct 11, 2024 at 1:04 AM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>
> The commit "3b5edc0421e2 (perf x86/topdown: Don't move topdown metric
> events in group)" modifies topdown metrics comparator to move topdown
> metrics events which are not in same group with previous event. But it
> just modifies the 2nd comparator and causes the comparators become
> asymmetric.
>
> Thus modify the 1st topdown metrics comparator and make the two
> comparators be symmetric, and refine the comments as well.
>
> Suggested-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Ian
> ---
> tools/perf/arch/x86/util/evlist.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index 438e4639fa89..447a734e591c 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -52,7 +52,7 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
> * 3,015,058 cycles
> * <not supported> topdown-retiring
> *
> - * if slots event and topdown metrics events are in two groups, the group which
> + * If slots event and topdown metrics events are in two groups, the group which
> * has topdown metrics events must contain only the topdown metrics event,
> * otherwise topdown metrics event can't be regrouped correctly as well, e.g.
> *
> @@ -69,13 +69,16 @@ int arch_evlist__cmp(const struct evsel *lhs, const struct evsel *rhs)
> return -1;
> if (arch_is_topdown_slots(rhs))
> return 1;
> - /* Followed by topdown events. */
> - if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs))
> - return -1;
> +
> /*
> - * Move topdown events forward only when topdown events
> - * are not in same group with previous event.
> + * Move topdown metrics events forward only when topdown metrics
> + * events are not in same group with previous slots event. If
> + * topdown metrics events are already in same group with slots
> + * event, do nothing.
> */
> + if (arch_is_topdown_metrics(lhs) && !arch_is_topdown_metrics(rhs) &&
> + lhs->core.leader != rhs->core.leader)
> + return -1;
> if (!arch_is_topdown_metrics(lhs) && arch_is_topdown_metrics(rhs) &&
> lhs->core.leader != rhs->core.leader)
> return 1;
>
> base-commit: 57fb40d40f9543213ffadb04d07cfb2ba46edc26
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] perf x86/topdown: Make topdown metrics comparators be symmetric
2024-10-11 11:02 [PATCH 1/2] perf x86/topdown: Make topdown metrics comparators be symmetric Dapeng Mi
2024-10-11 11:02 ` [PATCH 2/2] perf x86/topdown: Refine helper arch_is_topdown_metrics() Dapeng Mi
2024-10-16 17:56 ` [PATCH 1/2] perf x86/topdown: Make topdown metrics comparators be symmetric Ian Rogers
@ 2024-10-17 16:39 ` Namhyung Kim
2 siblings, 0 replies; 6+ messages in thread
From: Namhyung Kim @ 2024-10-17 16:39 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Ian Rogers,
Adrian Hunter, Alexander Shishkin, Kan Liang, Dapeng Mi
Cc: linux-perf-users, linux-kernel, Dapeng Mi
On Fri, 11 Oct 2024 11:02:06 +0000, Dapeng Mi wrote:
> The commit "3b5edc0421e2 (perf x86/topdown: Don't move topdown metric
> events in group)" modifies topdown metrics comparator to move topdown
> metrics events which are not in same group with previous event. But it
> just modifies the 2nd comparator and causes the comparators become
> asymmetric.
>
> Thus modify the 1st topdown metrics comparator and make the two
> comparators be symmetric, and refine the comments as well.
>
> [...]
Applied to perf-tools-next, thanks!
Best regards,
Namhyung
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-10-17 16:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-11 11:02 [PATCH 1/2] perf x86/topdown: Make topdown metrics comparators be symmetric Dapeng Mi
2024-10-11 11:02 ` [PATCH 2/2] perf x86/topdown: Refine helper arch_is_topdown_metrics() Dapeng Mi
2024-10-16 17:37 ` Namhyung Kim
2024-10-16 17:55 ` Ian Rogers
2024-10-16 17:56 ` [PATCH 1/2] perf x86/topdown: Make topdown metrics comparators be symmetric Ian Rogers
2024-10-17 16:39 ` Namhyung Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox