Re: [PATCH 14/17] coresight: Use per-sink trace ID maps for Perf sessions

From: Mike Leach
Date: Fri May 03 2024 - 05:44:25 EST


Hi James

On Mon, 29 Apr 2024 at 16:25, James Clark <james.clark@xxxxxxx> wrote:
>
> This will allow sessions with more than CORESIGHT_TRACE_IDS_MAX ETMs
> as long as there are fewer than that many ETMs connected to each sink.
>
> Each sink owns its own trace ID map, and any Perf session connecting to
> that sink will allocate from it, even if the sink is currently in use by
> other users. This is similar to the existing behavior where the dynamic
> trace IDs are constant as long as there is any concurrent Perf session
> active. It's not completely optimal because slightly more IDs will be
> used than necessary, but the optimal solution involves tracking the PIDs
> of each session and allocating ID maps based on the session owner. This
> is difficult to do with the combination of per-thread and per-cpu modes
> and some scheduling issues. The complexity of this isn't likely to worth
> it because even with multiple users they'd just see a difference in the
> ordering of ID allocations rather than hitting any limits (unless the
> hardware does have too many ETMs connected to one sink).
>
> Signed-off-by: James Clark <james.clark@xxxxxxx>
> ---
> drivers/hwtracing/coresight/coresight-core.c | 10 ++++++++++
> drivers/hwtracing/coresight/coresight-etm-perf.c | 15 ++++++++-------
> include/linux/coresight.h | 1 +
> 3 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 9fc6f6b863e0..d1adff467670 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -902,6 +902,7 @@ static void coresight_device_release(struct device *dev)
> struct coresight_device *csdev = to_coresight_device(dev);
>
> fwnode_handle_put(csdev->dev.fwnode);
> + free_percpu(csdev->perf_id_map.cpu_map);
> kfree(csdev);
> }
>
> @@ -1159,6 +1160,14 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
> csdev->dev.fwnode = fwnode_handle_get(dev_fwnode(desc->dev));
> dev_set_name(&csdev->dev, "%s", desc->name);
>
> + if (csdev->type == CORESIGHT_DEV_TYPE_SINK ||
> + csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) {
> + csdev->perf_id_map.cpu_map = alloc_percpu(atomic_t);
> + if (!csdev->perf_id_map.cpu_map) {
> + ret = -ENOMEM;
> + goto err_out;
> + }
> + }
> /*
> * Make sure the device registration and the connection fixup
> * are synchronised, so that we don't see uninitialised devices
> @@ -1216,6 +1225,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
> err_out:
> /* Cleanup the connection information */
> coresight_release_platform_data(NULL, desc->dev, desc->pdata);
> + kfree(csdev);
> return ERR_PTR(ret);
> }
> EXPORT_SYMBOL_GPL(coresight_register);
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 177cecae38d9..86ca1a9d09a7 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -229,10 +229,13 @@ static void free_event_data(struct work_struct *work)
> struct list_head **ppath;
>
> ppath = etm_event_cpu_path_ptr(event_data, cpu);
> - if (!(IS_ERR_OR_NULL(*ppath)))
> + if (!(IS_ERR_OR_NULL(*ppath))) {
> + struct coresight_device *sink = coresight_get_sink(*ppath);
> +
> + coresight_trace_id_put_cpu_id(cpu, &sink->perf_id_map);
> coresight_release_path(*ppath);
> + }
> *ppath = NULL;
> - coresight_trace_id_put_cpu_id(cpu, coresight_trace_id_map_default());
> }
>
> /* mark perf event as done for trace id allocator */
> @@ -401,8 +404,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
> }
>
> /* ensure we can allocate a trace ID for this CPU */
> - trace_id = coresight_trace_id_get_cpu_id(cpu,
> - coresight_trace_id_map_default());
> + trace_id = coresight_trace_id_get_cpu_id(cpu, &sink->perf_id_map);
> if (!IS_VALID_CS_TRACE_ID(trace_id)) {
> cpumask_clear_cpu(cpu, mask);
> coresight_release_path(path);
> @@ -497,7 +499,7 @@ static void etm_event_start(struct perf_event *event, int flags)
>
> /* Finally enable the tracer */
> if (source_ops(csdev)->enable(csdev, event, CS_MODE_PERF,
> - coresight_trace_id_map_default()))
> + &sink->perf_id_map))
> goto fail_disable_path;
>
> /*
> @@ -509,8 +511,7 @@ 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_map_default()));
> + coresight_trace_id_read_cpu_id(cpu, &sink->perf_id_map));
> perf_report_aux_output_id(event, hw_id);
> }
>
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 3a678e5425dc..8c4c1860c76b 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -290,6 +290,7 @@ struct coresight_device {
> bool sysfs_sink_activated;
> struct dev_ext_attribute *ea;
> struct coresight_device *def_sink;
> + struct coresight_trace_id_map perf_id_map;

perhaps this should be sink_id_map? At some point sysfs may use is and
naming is sink... is more consistent with other sink attributes in the
structure.

> /* sysfs links between components */
> int nr_links;
> bool has_conns_grp;
> --
> 2.34.1
>

Mike

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK