Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220602153603.1884710-1-zhengjun.xing@linux.intel.com>
In-Reply-To: <20220602153603.1884710-1-zhengjun.xing@linux.intel.com>
From:   Ian Rogers <irogers@google.com>
Date:   Thu, 2 Jun 2022 08:56:45 -0700
Message-ID: <CAP-5=fUw2TdA_nqbhCc7GjG6+XOfst6gW4FGAeshW7xxmq64nA@mail.gmail.com>
Subject: Re: [PATCH] perf record: Support sample-read topdown metric group for
 hybrid platforms
To:     zhengjun.xing@linux.intel.com
Cc:     acme@kernel.org, peterz@infradead.org, mingo@redhat.com,
        alexander.shishkin@intel.com, jolsa@redhat.com,
        linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        adrian.hunter@intel.com, ak@linux.intel.com,
        kan.liang@linux.intel.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jun 2, 2022 at 8:35 AM <zhengjun.xing@linux.intel.com> wrote:
>
> From: Zhengjun Xing <zhengjun.xing@linux.intel.com>
>
> With the hardware TopDown metrics feature, the sample-read feature should
> be supported for a TopDown group, e.g., sample a non-topdown event and read
> a Topdown metric group. But the current perf record code errors are out.
>
> For a TopDown metric group,the slots event must be the leader of the group,
> but the leader slots event doesn't support sampling. To support sample-read
> the TopDown metric group, uses the 2nd event of the group as the "leader"
> for the purposes of sampling.
>
> Only the platform with the TopDown metric feature supports sample-read the
> topdown group. In commit acb65150a47c ("perf record: Support sample-read
> topdown metric group"), it adds arch_topdown_sample_read() to indicate
> whether the TopDown group supports sample-read, it should only work on the
> non-hybrid systems, this patch extends the support for hybrid platforms.
>
> Before:
>
>  # ./perf record -e "{cpu_core/slots/,cpu_core/cycles/,cpu_core/topdown-retiring/}:S" -a sleep 1
> Error:
> The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (cpu_core/topdown-retiring/).
> /bin/dmesg | grep -i perf may provide additional information.
>
> After:
>
>  # ./perf record -e "{cpu_core/slots/,cpu_core/cycles/,cpu_core/topdown-retiring/}:S" -a sleep 1
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.238 MB perf.data (369 samples) ]
>
> Fixes: acb65150a47c ("perf record: Support sample-read topdown metric group")
> Signed-off-by: Zhengjun Xing <zhengjun.xing@linux.intel.com>
> Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/arch/x86/util/evsel.c   |  3 ++-
>  tools/perf/arch/x86/util/evsel.h   |  7 +++++++
>  tools/perf/arch/x86/util/topdown.c | 21 ++++-----------------
>  3 files changed, 13 insertions(+), 18 deletions(-)
>  create mode 100644 tools/perf/arch/x86/util/evsel.h
>
> diff --git a/tools/perf/arch/x86/util/evsel.c b/tools/perf/arch/x86/util/evsel.c
> index ff4561b7b600..3501399cef35 100644
> --- a/tools/perf/arch/x86/util/evsel.c
> +++ b/tools/perf/arch/x86/util/evsel.c
> @@ -5,6 +5,7 @@
>  #include "util/env.h"
>  #include "util/pmu.h"
>  #include "linux/string.h"
> +#include "evsel.h"
>
>  void arch_evsel__set_sample_weight(struct evsel *evsel)
>  {
> @@ -32,7 +33,7 @@ void arch_evsel__fixup_new_cycles(struct perf_event_attr *attr)
>  }
>
>  /* Check whether the evsel's PMU supports the perf metrics */
> -static bool evsel__sys_has_perf_metrics(const struct evsel *evsel)
> +bool evsel__sys_has_perf_metrics(const struct evsel *evsel)
>  {
>         const char *pmu_name = evsel->pmu_name ? evsel->pmu_name : "cpu";
>
> diff --git a/tools/perf/arch/x86/util/evsel.h b/tools/perf/arch/x86/util/evsel.h
> new file mode 100644
> index 000000000000..19ad1691374d
> --- /dev/null
> +++ b/tools/perf/arch/x86/util/evsel.h
> @@ -0,0 +1,7 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _EVSEL_H
> +#define _EVSEL_H 1
> +
> +bool evsel__sys_has_perf_metrics(const struct evsel *evsel);
> +
> +#endif
> diff --git a/tools/perf/arch/x86/util/topdown.c b/tools/perf/arch/x86/util/topdown.c
> index f4d5422e9960..f81a7cfe4d63 100644
> --- a/tools/perf/arch/x86/util/topdown.c
> +++ b/tools/perf/arch/x86/util/topdown.c
> @@ -4,6 +4,7 @@
>  #include "util/pmu.h"
>  #include "util/topdown.h"
>  #include "topdown.h"
> +#include "evsel.h"
>
>  /* Check whether there is a PMU which supports the perf metrics. */
>  bool topdown_sys_has_perf_metrics(void)
> @@ -55,33 +56,19 @@ void arch_topdown_group_warn(void)
>
>  #define TOPDOWN_SLOTS          0x0400
>
> -static bool is_topdown_slots_event(struct evsel *counter)
> -{
> -       if (!counter->pmu_name)
> -               return false;
> -
> -       if (strcmp(counter->pmu_name, "cpu"))
> -               return false;
> -
> -       if (counter->core.attr.config == TOPDOWN_SLOTS)
> -               return true;
> -
> -       return false;
> -}
> -
>  /*
>   * Check whether a topdown group supports sample-read.
>   *
> - * Only Topdown metic supports sample-read. The slots
> + * Only Topdown metric supports sample-read. The slots
>   * event must be the leader of the topdown group.
>   */
>
>  bool arch_topdown_sample_read(struct evsel *leader)
>  {
> -       if (!pmu_have_event("cpu", "slots"))
> +       if (!evsel__sys_has_perf_metrics(leader))
>                 return false;
>
> -       if (is_topdown_slots_event(leader))
> +       if (leader->core.attr.config == TOPDOWN_SLOTS)
>                 return true;
>
>         return false;
> --
> 2.25.1
>