Received-SPF: pass (google.com: domain of linux-kernel+bounces-69620-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240202234057.2085863-1-irogers@google.com> <20240202234057.2085863-4-irogers@google.com>
In-Reply-To: <20240202234057.2085863-4-irogers@google.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Fri, 16 Feb 2024 17:01:45 -0800
Message-ID: <CAM9d7chPqFGEih7z7rp=eS5P30gSMvG=6fi=0QqT=EdfdMOH_A@mail.gmail.com>
Subject: Re: [PATCH v3 3/8] perf arm-spe/cs-etm: Directly iterate CPU maps
To: Ian Rogers <irogers@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, Mark Rutland <mark.rutland@arm.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, 
	Mike Leach <mike.leach@linaro.org>, James Clark <james.clark@arm.com>, 
	John Garry <john.g.garry@oracle.com>, Will Deacon <will@kernel.org>, 
	Thomas Gleixner <tglx@linutronix.de>, Darren Hart <dvhart@infradead.org>, 
	Davidlohr Bueso <dave@stgolabs.net>, =?UTF-8?Q?Andr=C3=A9_Almeida?= <andrealmeid@igalia.com>, 
	Kan Liang <kan.liang@linux.intel.com>, K Prateek Nayak <kprateek.nayak@amd.com>, 
	Sean Christopherson <seanjc@google.com>, Paolo Bonzini <pbonzini@redhat.com>, 
	Kajol Jain <kjain@linux.ibm.com>, Athira Rajeev <atrajeev@linux.vnet.ibm.com>, 
	Andrew Jones <ajones@ventanamicro.com>, Alexandre Ghiti <alexghiti@rivosinc.com>, 
	Atish Patra <atishp@rivosinc.com>, "Steinar H. Gunderson" <sesse@google.com>, 
	Yang Jihong <yangjihong1@huawei.com>, Yang Li <yang.lee@linux.alibaba.com>, 
	Changbin Du <changbin.du@huawei.com>, Sandipan Das <sandipan.das@amd.com>, 
	Ravi Bangoria <ravi.bangoria@amd.com>, Paran Lee <p4ranlee@gmail.com>, 
	Nick Desaulniers <ndesaulniers@google.com>, Huacai Chen <chenhuacai@kernel.org>, 
	Yanteng Si <siyanteng@loongson.cn>, linux-perf-users@vger.kernel.org, 
	linux-kernel@vger.kernel.org, coresight@lists.linaro.org, 
	linux-arm-kernel@lists.infradead.org, bpf@vger.kernel.org, 
	Leo Yan <leo.yan@linux.dev>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 2, 2024 at 3:41=E2=80=AFPM Ian Rogers <irogers@google.com> wrot=
e:
>
> Rather than iterate all CPUs and see if they are in CPU maps, directly
> iterate the CPU map. Similarly make use of the intersect function
> taking care for when "any" CPU is specified. Switch
> perf_cpu_map__has_any_cpu_or_is_empty to more appropriate
> alternatives.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/arm/util/cs-etm.c    | 114 ++++++++++++---------------
>  tools/perf/arch/arm64/util/arm-spe.c |   4 +-
>  2 files changed, 51 insertions(+), 67 deletions(-)
>
> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util=
/cs-etm.c
> index 77e6663c1703..07be32d99805 100644
> --- a/tools/perf/arch/arm/util/cs-etm.c
> +++ b/tools/perf/arch/arm/util/cs-etm.c
> @@ -197,38 +197,37 @@ static int cs_etm_validate_timestamp(struct auxtrac=
e_record *itr,
>  static int cs_etm_validate_config(struct auxtrace_record *itr,
>                                   struct evsel *evsel)
>  {
> -       int i, err =3D -EINVAL;
> +       int idx, err =3D 0;
>         struct perf_cpu_map *event_cpus =3D evsel->evlist->core.user_requ=
ested_cpus;
> -       struct perf_cpu_map *online_cpus =3D perf_cpu_map__new_online_cpu=
s();
> -
> -       /* Set option of each CPU we have */
> -       for (i =3D 0; i < cpu__max_cpu().cpu; i++) {
> -               struct perf_cpu cpu =3D { .cpu =3D i, };
> +       struct perf_cpu_map *intersect_cpus;
> +       struct perf_cpu cpu;
>
> -               /*
> -                * In per-cpu case, do the validation for CPUs to work wi=
th.
> -                * In per-thread case, the CPU map is empty.  Since the t=
raced
> -                * program can run on any CPUs in this case, thus don't s=
kip
> -                * validation.
> -                */
> -               if (!perf_cpu_map__has_any_cpu_or_is_empty(event_cpus) &&
> -                   !perf_cpu_map__has(event_cpus, cpu))
> -                       continue;
> +       /*
> +        * Set option of each CPU we have. In per-cpu case, do the valida=
tion
> +        * for CPUs to work with. In per-thread case, the CPU map has the=
 "any"
> +        * CPU value. Since the traced program can run on any CPUs in thi=
s case,
> +        * thus don't skip validation.
> +        */
> +       if (!perf_cpu_map__has_any_cpu(event_cpus)) {
> +               struct perf_cpu_map *online_cpus =3D perf_cpu_map__new_on=
line_cpus();
>
> -               if (!perf_cpu_map__has(online_cpus, cpu))
> -                       continue;
> +               intersect_cpus =3D perf_cpu_map__intersect(event_cpus, on=
line_cpus);
> +               perf_cpu_map__put(online_cpus);
> +       } else {
> +               intersect_cpus =3D perf_cpu_map__new_online_cpus();
> +       }

Would it be ok if any of these operations fail?  I believe the
cpu map functions work well with NULL already.

Thanks,
Namhyung

>
> -               err =3D cs_etm_validate_context_id(itr, evsel, i);
> +       perf_cpu_map__for_each_cpu_skip_any(cpu, idx, intersect_cpus) {
> +               err =3D cs_etm_validate_context_id(itr, evsel, cpu.cpu);
>                 if (err)
> -                       goto out;
> -               err =3D cs_etm_validate_timestamp(itr, evsel, i);
> +                       break;
> +
> +               err =3D cs_etm_validate_timestamp(itr, evsel, cpu.cpu);
>                 if (err)
> -                       goto out;
> +                       break;
>         }
>
> -       err =3D 0;
> -out:
> -       perf_cpu_map__put(online_cpus);
> +       perf_cpu_map__put(intersect_cpus);
>         return err;
>  }
>
> @@ -435,7 +434,7 @@ static int cs_etm_recording_options(struct auxtrace_r=
ecord *itr,
>          * Also the case of per-cpu mmaps, need the contextID in order to=
 be notified
>          * when a context switch happened.
>          */
> -       if (!perf_cpu_map__has_any_cpu_or_is_empty(cpus)) {
> +       if (!perf_cpu_map__is_any_cpu_or_is_empty(cpus)) {
>                 evsel__set_config_if_unset(cs_etm_pmu, cs_etm_evsel,
>                                            "timestamp", 1);
>                 evsel__set_config_if_unset(cs_etm_pmu, cs_etm_evsel,
> @@ -461,7 +460,7 @@ static int cs_etm_recording_options(struct auxtrace_r=
ecord *itr,
>         evsel->core.attr.sample_period =3D 1;
>
>         /* In per-cpu case, always need the time of mmap events etc */
> -       if (!perf_cpu_map__has_any_cpu_or_is_empty(cpus))
> +       if (!perf_cpu_map__is_any_cpu_or_is_empty(cpus))
>                 evsel__set_sample_bit(evsel, TIME);
>
>         err =3D cs_etm_validate_config(itr, cs_etm_evsel);
> @@ -533,45 +532,31 @@ static size_t
>  cs_etm_info_priv_size(struct auxtrace_record *itr __maybe_unused,
>                       struct evlist *evlist __maybe_unused)
>  {
> -       int i;
> +       int idx;
>         int etmv3 =3D 0, etmv4 =3D 0, ete =3D 0;
>         struct perf_cpu_map *event_cpus =3D evlist->core.user_requested_c=
pus;
> -       struct perf_cpu_map *online_cpus =3D perf_cpu_map__new_online_cpu=
s();
> -
> -       /* cpu map is not empty, we have specific CPUs to work with */
> -       if (!perf_cpu_map__has_any_cpu_or_is_empty(event_cpus)) {
> -               for (i =3D 0; i < cpu__max_cpu().cpu; i++) {
> -                       struct perf_cpu cpu =3D { .cpu =3D i, };
> +       struct perf_cpu_map *intersect_cpus;
> +       struct perf_cpu cpu;
>
> -                       if (!perf_cpu_map__has(event_cpus, cpu) ||
> -                           !perf_cpu_map__has(online_cpus, cpu))
> -                               continue;
> +       if (!perf_cpu_map__has_any_cpu(event_cpus)) {
> +               /* cpu map is not "any" CPU , we have specific CPUs to wo=
rk with */
> +               struct perf_cpu_map *online_cpus =3D perf_cpu_map__new_on=
line_cpus();
>
> -                       if (cs_etm_is_ete(itr, i))
> -                               ete++;
> -                       else if (cs_etm_is_etmv4(itr, i))
> -                               etmv4++;
> -                       else
> -                               etmv3++;
> -               }
> +               intersect_cpus =3D perf_cpu_map__intersect(event_cpus, on=
line_cpus);
> +               perf_cpu_map__put(online_cpus);
>         } else {
> -               /* get configuration for all CPUs in the system */
> -               for (i =3D 0; i < cpu__max_cpu().cpu; i++) {
> -                       struct perf_cpu cpu =3D { .cpu =3D i, };
> -
> -                       if (!perf_cpu_map__has(online_cpus, cpu))
> -                               continue;
> -
> -                       if (cs_etm_is_ete(itr, i))
> -                               ete++;
> -                       else if (cs_etm_is_etmv4(itr, i))
> -                               etmv4++;
> -                       else
> -                               etmv3++;
> -               }
> +               /* Event can be "any" CPU so count all online CPUs. */
> +               intersect_cpus =3D perf_cpu_map__new_online_cpus();
>         }
> -
> -       perf_cpu_map__put(online_cpus);
> +       perf_cpu_map__for_each_cpu_skip_any(cpu, idx, intersect_cpus) {
> +               if (cs_etm_is_ete(itr, cpu.cpu))
> +                       ete++;
> +               else if (cs_etm_is_etmv4(itr, cpu.cpu))
> +                       etmv4++;
> +               else
> +                       etmv3++;
> +       }
> +       perf_cpu_map__put(intersect_cpus);
>
>         return (CS_ETM_HEADER_SIZE +
>                (ete   * CS_ETE_PRIV_SIZE) +
> @@ -813,16 +798,15 @@ static int cs_etm_info_fill(struct auxtrace_record =
*itr,
>         if (!session->evlist->core.nr_mmaps)
>                 return -EINVAL;
>
> -       /* If the cpu_map is empty all online CPUs are involved */
> -       if (perf_cpu_map__has_any_cpu_or_is_empty(event_cpus)) {
> +       /* If the cpu_map has the "any" CPU all online CPUs are involved =
*/
> +       if (perf_cpu_map__has_any_cpu(event_cpus)) {
>                 cpu_map =3D online_cpus;
>         } else {
>                 /* Make sure all specified CPUs are online */
> -               for (i =3D 0; i < perf_cpu_map__nr(event_cpus); i++) {
> -                       struct perf_cpu cpu =3D { .cpu =3D i, };
> +               struct perf_cpu cpu;
>
> -                       if (perf_cpu_map__has(event_cpus, cpu) &&
> -                           !perf_cpu_map__has(online_cpus, cpu))
> +               perf_cpu_map__for_each_cpu(cpu, i, event_cpus) {
> +                       if (!perf_cpu_map__has(online_cpus, cpu))
>                                 return -EINVAL;
>                 }
>
> diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64=
/util/arm-spe.c
> index 51ccbfd3d246..0b52e67edb3b 100644
> --- a/tools/perf/arch/arm64/util/arm-spe.c
> +++ b/tools/perf/arch/arm64/util/arm-spe.c
> @@ -232,7 +232,7 @@ static int arm_spe_recording_options(struct auxtrace_=
record *itr,
>          * In the case of per-cpu mmaps, sample CPU for AUX event;
>          * also enable the timestamp tracing for samples correlation.
>          */
> -       if (!perf_cpu_map__has_any_cpu_or_is_empty(cpus)) {
> +       if (!perf_cpu_map__is_any_cpu_or_is_empty(cpus)) {
>                 evsel__set_sample_bit(arm_spe_evsel, CPU);
>                 evsel__set_config_if_unset(arm_spe_pmu, arm_spe_evsel,
>                                            "ts_enable", 1);
> @@ -265,7 +265,7 @@ static int arm_spe_recording_options(struct auxtrace_=
record *itr,
>         tracking_evsel->core.attr.sample_period =3D 1;
>
>         /* In per-cpu case, always need the time of mmap events etc */
> -       if (!perf_cpu_map__has_any_cpu_or_is_empty(cpus)) {
> +       if (!perf_cpu_map__is_any_cpu_or_is_empty(cpus)) {
>                 evsel__set_sample_bit(tracking_evsel, TIME);
>                 evsel__set_sample_bit(tracking_evsel, CPU);
>
> --
> 2.43.0.594.gd9cf4e227d-goog
>