Received-SPF: pass (google.com: domain of linux-kernel+bounces-48009-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <9d177173-c66f-a0d3-ba4d-2261f8663fce@arm.com>
Date: Thu, 1 Feb 2024 11:25:47 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH v2 3/8] perf arm-spe/cs-etm: Directly iterate CPU maps
Content-Language: en-US
To: Ian Rogers <irogers@google.com>
References: <20240201042236.1538928-1-irogers@google.com>
 <20240201042236.1538928-4-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>, Namhyung Kim <namhyung@kernel.org>,
 Adrian Hunter <adrian.hunter@intel.com>,
 Suzuki K Poulose <suzuki.poulose@arm.com>, Mike Leach
 <mike.leach@linaro.org>, Leo Yan <leo.yan@linaro.org>,
 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
From: James Clark <james.clark@arm.com>
In-Reply-To: <20240201042236.1538928-4-irogers@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 01/02/2024 04:22, Ian Rogers 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. 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    | 77 ++++++++++++----------------
>  tools/perf/arch/arm64/util/arm-spe.c |  4 +-
>  2 files changed, 34 insertions(+), 47 deletions(-)
> 
> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> index 77e6663c1703..f4378ba0b8d6 100644
> --- a/tools/perf/arch/arm/util/cs-etm.c
> +++ b/tools/perf/arch/arm/util/cs-etm.c
> @@ -197,38 +197,32 @@ 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 = -EINVAL;
>  	struct perf_cpu_map *event_cpus = evsel->evlist->core.user_requested_cpus;
>  	struct perf_cpu_map *online_cpus = perf_cpu_map__new_online_cpus();
> +	struct perf_cpu_map *intersect_cpus = perf_cpu_map__intersect(event_cpus, online_cpus);

Hi Ian,

This has the same issue as V1. 'any' intersect 'online' == empty. Now no
validation happens anymore. For this to be the same as it used to be,
validation has to happen on _all_ cores when event_cpus == -1. So it
needs to be 'any' intersect 'online' == 'online'.

Same issue below with cs_etm_info_priv_size()

Thanks
James

> +	struct perf_cpu cpu;
>  
> -	/* Set option of each CPU we have */
> -	for (i = 0; i < cpu__max_cpu().cpu; i++) {
> -		struct perf_cpu cpu = { .cpu = i, };
> -
> -		/*
> -		 * 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;
> -
> -		if (!perf_cpu_map__has(online_cpus, cpu))
> -			continue;
> +	perf_cpu_map__put(online_cpus);
>  
> -		err = cs_etm_validate_context_id(itr, evsel, i);
> +	/*
> +	 * 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 is empty.
> +	 * Since the traced program can run on any CPUs in this case, thus don't
> +	 * skip validation.
> +	 */
> +	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);
> +		err = cs_etm_validate_timestamp(itr, evsel, cpu.cpu);
>  		if (err)
>  			goto out;
>  	}
>  
>  	err = 0;
>  out:
> -	perf_cpu_map__put(online_cpus);
> +	perf_cpu_map__put(intersect_cpus);
>  	return err;
>  }
>  
> @@ -435,7 +429,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 +455,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,38 +527,32 @@ 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();
> +	struct perf_cpu cpu;
>  
>  	/* 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, };
> -
> -			if (!perf_cpu_map__has(event_cpus, cpu) ||
> -			    !perf_cpu_map__has(online_cpus, cpu))
> -				continue;
> +	if (!perf_cpu_map__is_empty(event_cpus)) {
> +		struct perf_cpu_map *intersect_cpus =
> +			perf_cpu_map__intersect(event_cpus, online_cpus);
>  
> -			if (cs_etm_is_ete(itr, i))
> +		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, i))
> +			else if (cs_etm_is_etmv4(itr, cpu.cpu))
>  				etmv4++;
>  			else
>  				etmv3++;
>  		}
> +		perf_cpu_map__put(intersect_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))
> +		perf_cpu_map__for_each_cpu(cpu, idx, online_cpus) {
> +			if (cs_etm_is_ete(itr, cpu.cpu))
>  				ete++;
> -			else if (cs_etm_is_etmv4(itr, i))
> +			else if (cs_etm_is_etmv4(itr, cpu.cpu))
>  				etmv4++;
>  			else
>  				etmv3++;
> @@ -814,15 +802,14 @@ static int cs_etm_info_fill(struct auxtrace_record *itr,
>  		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 (perf_cpu_map__is_empty(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);
>