Re: [PATCH 12/17] coresight: Make CPU id map a property of a trace ID map

From: James Clark
Date: Tue May 07 2024 - 05:59:21 EST




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@xxxxxxx>
>> ---
>> .../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.