Re: [PATCH v3 3/8] perf arm-spe/cs-etm: Directly iterate CPU maps

[Namhyung Kim]

On Fri, Feb 2, 2024 at 3:41 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> ---
>  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 auxtrace_record *itr,
>  static int cs_etm_validate_config(struct auxtrace_record *itr,
>                                   struct evsel *evsel)
>  {
> -       int i, err = -EINVAL;
> +       int idx, err = 0;
>         struct perf_cpu_map *event_cpus = evsel->evlist->core.user_requested_cpus;
> -       struct perf_cpu_map *online_cpus = perf_cpu_map__new_online_cpus();
> -
> -       /* Set option of each CPU we have */
> -       for (i = 0; i < cpu__max_cpu().cpu; i++) {
> -               struct perf_cpu cpu = { .cpu = i, };
> +       struct perf_cpu_map *intersect_cpus;
> +       struct perf_cpu cpu;
>
> -               /*
> -                * In per-cpu case, do the validation for CPUs to work with.
> -                * In per-thread case, the CPU map is empty.  Since the traced
> -                * program can run on any CPUs in this case, thus don't skip
> -                * 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 validation
> +        * 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 this case,
> +        * thus don't skip validation.
> +        */
> +       if (!perf_cpu_map__has_any_cpu(event_cpus)) {
> +               struct perf_cpu_map *online_cpus = perf_cpu_map__new_online_cpus();
>
> -               if (!perf_cpu_map__has(online_cpus, cpu))
> -                       continue;
> +               intersect_cpus = perf_cpu_map__intersect(event_cpus, online_cpus);
> +               perf_cpu_map__put(online_cpus);
> +       } else {
> +               intersect_cpus = 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 = cs_etm_validate_context_id(itr, evsel, i);
> +       perf_cpu_map__for_each_cpu_skip_any(cpu, idx, intersect_cpus) {
> +               err = cs_etm_validate_context_id(itr, evsel, cpu.cpu);
>                 if (err)
> -                       goto out;
> -               err = cs_etm_validate_timestamp(itr, evsel, i);
> +                       break;
> +
> +               err = cs_etm_validate_timestamp(itr, evsel, cpu.cpu);
>                 if (err)
> -                       goto out;
> +                       break;
>         }
>
> -       err = 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_record *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_record *itr,
>         evsel->core.attr.sample_period = 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 = 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 = 0, etmv4 = 0, ete = 0;
>         struct perf_cpu_map *event_cpus = evlist->core.user_requested_cpus;
> -       struct perf_cpu_map *online_cpus = perf_cpu_map__new_online_cpus();
> -
> -       /* 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 = 0; i < cpu__max_cpu().cpu; i++) {
> -                       struct perf_cpu cpu = { .cpu = 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 work with */
> +               struct perf_cpu_map *online_cpus = perf_cpu_map__new_online_cpus();
>
> -                       if (cs_etm_is_ete(itr, i))
> -                               ete++;
> -                       else if (cs_etm_is_etmv4(itr, i))
> -                               etmv4++;
> -                       else
> -                               etmv3++;
> -               }
> +               intersect_cpus = perf_cpu_map__intersect(event_cpus, online_cpus);
> +               perf_cpu_map__put(online_cpus);
>         } else {
> -               /* get configuration for all CPUs in the system */
> -               for (i = 0; i < cpu__max_cpu().cpu; i++) {
> -                       struct perf_cpu cpu = { .cpu = 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 = 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 = online_cpus;
>         } else {
>                 /* Make sure all specified CPUs are online */
> -               for (i = 0; i < perf_cpu_map__nr(event_cpus); i++) {
> -                       struct perf_cpu cpu = { .cpu = 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 = 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
>