Date: Tue, 7 May 2024 10:57:46 +0100
From: James Clark <james.clark@....com>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
 Adrian Hunter <adrian.hunter@...el.com>, John Garry
 <john.g.garry@...cle.com>, Will Deacon <will@...nel.org>,
 Leo Yan <leo.yan@...ux.dev>, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com,
 linux-perf-users@...r.kernel.org, gankulkarni@...amperecomputing.com,
 scclevenger@...amperecomputing.com, coresight@...ts.linaro.org,
 suzuki.poulose@....com, mike.leach@...aro.org
Subject: Re: [PATCH 12/17] coresight: Make CPU id map a property of a trace ID
 map



On 07/05/2024 07:22, Anshuman Khandual wrote:
> 
> 
> On 4/29/24 20:51, James Clark wrote:
>> The global CPU ID mappings won't work for per-sink ID maps so move it to
>> the ID map struct. coresight_trace_id_release_all_pending() is hard
>> coded to operate on the default map, but once Perf sessions use their
>> own maps the pending release mechanism will be deleted. So it doesn't
>> need to be extended to accept a trace ID map argument at this point.
>>
>> Signed-off-by: James Clark <james.clark@....com>
>> ---
>>  .../hwtracing/coresight/coresight-etm-perf.c  |  3 +-
>>  .../coresight/coresight-etm3x-core.c          |  3 +-
>>  .../coresight/coresight-etm4x-core.c          |  3 +-
>>  .../hwtracing/coresight/coresight-trace-id.c  | 28 ++++++++-----------
>>  .../hwtracing/coresight/coresight-trace-id.h  |  2 +-
>>  include/linux/coresight.h                     |  1 +
>>  6 files changed, 20 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> index 4afb9d29f355..25f1f87c90d1 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>> @@ -508,7 +508,8 @@ static void etm_event_start(struct perf_event *event, int flags)
>>  		hw_id = FIELD_PREP(CS_AUX_HW_ID_VERSION_MASK,
>>  				   CS_AUX_HW_ID_CURR_VERSION);
>>  		hw_id |= FIELD_PREP(CS_AUX_HW_ID_TRACE_ID_MASK,
>> -				    coresight_trace_id_read_cpu_id(cpu));
>> +				    coresight_trace_id_read_cpu_id(cpu,
>> +						coresight_trace_id_map_default()));
>>  		perf_report_aux_output_id(event, hw_id);
>>  	}
>>  
>> diff --git a/drivers/hwtracing/coresight/coresight-etm3x-core.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
>> index 4149e7675ceb..b21f5ad94e63 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm3x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
>> @@ -501,7 +501,8 @@ static int etm_enable_perf(struct coresight_device *csdev,
>>  	 * with perf locks - we know the ID cannot change until perf shuts down
>>  	 * the session
>>  	 */
>> -	trace_id = coresight_trace_id_read_cpu_id(drvdata->cpu);
>> +	trace_id = coresight_trace_id_read_cpu_id(drvdata->cpu,
>> +						  coresight_trace_id_map_default());
>>  	if (!IS_VALID_CS_TRACE_ID(trace_id)) {
>>  		dev_err(&drvdata->csdev->dev, "Failed to set trace ID for %s on CPU%d\n",
>>  			dev_name(&drvdata->csdev->dev), drvdata->cpu);
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index f32c8cd7742d..d16d6efb26fa 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -776,7 +776,8 @@ static int etm4_enable_perf(struct coresight_device *csdev,
>>  	 * with perf locks - we know the ID cannot change until perf shuts down
>>  	 * the session
>>  	 */
>> -	trace_id = coresight_trace_id_read_cpu_id(drvdata->cpu);
>> +	trace_id = coresight_trace_id_read_cpu_id(drvdata->cpu,
>> +						  coresight_trace_id_map_default());
>>  	if (!IS_VALID_CS_TRACE_ID(trace_id)) {
>>  		dev_err(&drvdata->csdev->dev, "Failed to set trace ID for %s on CPU%d\n",
>>  			dev_name(&drvdata->csdev->dev), drvdata->cpu);
>> diff --git a/drivers/hwtracing/coresight/coresight-trace-id.c b/drivers/hwtracing/coresight/coresight-trace-id.c
>> index 45ddd50d09a6..b393603dd713 100644
>> --- a/drivers/hwtracing/coresight/coresight-trace-id.c
>> +++ b/drivers/hwtracing/coresight/coresight-trace-id.c
>> @@ -13,10 +13,12 @@
>>  #include "coresight-trace-id.h"
>>  
>>  /* Default trace ID map. Used in sysfs mode and for system sources */
>> -static struct coresight_trace_id_map id_map_default;
>> +static DEFINE_PER_CPU(atomic_t, id_map_default_cpu_ids) = ATOMIC_INIT(0);
>> +static struct coresight_trace_id_map id_map_default = {
>> +	.cpu_map = &id_map_default_cpu_ids
>> +};
>>  
>> -/* maintain a record of the mapping of IDs and pending releases per cpu */
>> -static DEFINE_PER_CPU(atomic_t, cpu_id) = ATOMIC_INIT(0);
>> +/* maintain a record of the pending releases per cpu */
>>  static cpumask_t cpu_id_release_pending;
>>  
>>  /* perf session active counter */
>> @@ -46,12 +48,6 @@ static void coresight_trace_id_dump_table(struct coresight_trace_id_map *id_map,
>>  #define PERF_SESSION(n)
>>  #endif
>>  
>> -/* unlocked read of current trace ID value for given CPU */
>> -static int _coresight_trace_id_read_cpu_id(int cpu)
>> -{
>> -	return atomic_read(&per_cpu(cpu_id, cpu));
>> -}
> 
> Just wondering where this per cpu cpu_id ^^ is being dropped off as well
> OR is it still getting used ?
> 

No it's still needed. It's not dropped in this set.

I just moved the implementation into coresight_trace_id_read_cpu_id()
rather than add a new argument to _coresight_trace_id_read_cpu_id().
Although I might change that because of Mike's comment about keeping
both the map and non map versions of the functions to reduce some of the
churn changing the callers.

Powered by blists - more mailing lists