Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC 02/11] coresight: etm-perf: Allow an event to use different
 sinks
To:     Anshuman Khandual <anshuman.khandual@arm.com>,
        linux-arm-kernel@lists.infradead.org, coresight@lists.linaro.org
Cc:     linux-kernel@vger.kernel.org, mathieu.poirier@linaro.org,
        mike.leach@linaro.org, Linu Cherian <lcherian@marvell.com>,
        Linu Cherian <linuc.decode@gmail.com>
References: <1605012309-24812-1-git-send-email-anshuman.khandual@arm.com>
 <1605012309-24812-3-git-send-email-anshuman.khandual@arm.com>
From:   Suzuki K Poulose <suzuki.poulose@arm.com>
Message-ID: <67e0864f-e025-aa08-d1b7-36cf19629197@arm.com>
Date:   Thu, 12 Nov 2020 09:21:40 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.1
MIME-Version: 1.0
In-Reply-To: <1605012309-24812-3-git-send-email-anshuman.khandual@arm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Linu,

Please could you test this slightly modified version and give us
a Tested-by tag if you are happy with the results ?

Suzuki


On 11/10/20 12:45 PM, Anshuman Khandual wrote:
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> When there are multiple sinks on the system, in the absence
> of a specified sink, it is quite possible that a default sink
> for an ETM could be different from that of another ETM. However
> we do not support having multiple sinks for an event yet. This
> patch allows the event to use the default sinks on the ETMs
> where they are scheduled as long as the sinks are of the same
> type.
> 
> e.g, if we have 1x1 topology with per-CPU ETRs, the event can
> use the per-CPU ETR for the session. However, if the sinks
> are of different type, e.g TMC-ETR on one and a custom sink
> on another, the event will only trace on the first detected
> sink.
> 
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-etm-perf.c | 50 ++++++++++++++++++------
>   1 file changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index c2c9b12..ea73cfa 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -204,14 +204,22 @@ static void etm_free_aux(void *data)
>   	schedule_work(&event_data->work);
>   }
>   
> +static bool sinks_match(struct coresight_device *a, struct coresight_device *b)
> +{
> +	if (!a || !b)
> +		return false;
> +	return (sink_ops(a) == sink_ops(b));
> +}
> +
>   static void *etm_setup_aux(struct perf_event *event, void **pages,
>   			   int nr_pages, bool overwrite)
>   {
>   	u32 id;
>   	int cpu = event->cpu;
>   	cpumask_t *mask;
> -	struct coresight_device *sink;
> +	struct coresight_device *sink = NULL;
>   	struct etm_event_data *event_data = NULL;
> +	bool sink_forced = false;
>   
>   	event_data = alloc_event_data(cpu);
>   	if (!event_data)
> @@ -222,6 +230,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>   	if (event->attr.config2) {
>   		id = (u32)event->attr.config2;
>   		sink = coresight_get_sink_by_id(id);
> +		sink_forced = true;
>   	}
>   
>   	mask = &event_data->mask;
> @@ -235,7 +244,7 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>   	 */
>   	for_each_cpu(cpu, mask) {
>   		struct list_head *path;
> -		struct coresight_device *csdev;
> +		struct coresight_device *csdev, *new_sink;
>   
>   		csdev = per_cpu(csdev_src, cpu);
>   		/*
> @@ -249,21 +258,35 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>   		}
>   
>   		/*
> -		 * No sink provided - look for a default sink for one of the
> -		 * devices. At present we only support topology where all CPUs
> -		 * use the same sink [N:1], so only need to find one sink. The
> -		 * coresight_build_path later will remove any CPU that does not
> -		 * attach to the sink, or if we have not found a sink.
> +		 * No sink provided - look for a default sink for all the devices.
> +		 * We only support multiple sinks, only if all the default sinks
> +		 * are of the same type, so that the sink buffer can be shared
> +		 * as the event moves around. We don't trace on a CPU if it can't
> +		 *
>   		 */
> -		if (!sink)
> -			sink = coresight_find_default_sink(csdev);
> +		if (!sink_forced) {
> +			new_sink = coresight_find_default_sink(csdev);
> +			if (!new_sink) {
> +				cpumask_clear_cpu(cpu, mask);
> +				continue;
> +			}
> +			/* Skip checks for the first sink */
> +			if (!sink) {
> +				sink = new_sink;
> +			} else if (!sinks_match(new_sink, sink)) {
> +				cpumask_clear_cpu(cpu, mask);
> +				continue;
> +			}
> +		} else {
> +			new_sink = sink;
> +		}
>   
>   		/*
>   		 * Building a path doesn't enable it, it simply builds a
>   		 * list of devices from source to sink that can be
>   		 * referenced later when the path is actually needed.
>   		 */
> -		path = coresight_build_path(csdev, sink);
> +		path = coresight_build_path(csdev, new_sink);
>   		if (IS_ERR(path)) {
>   			cpumask_clear_cpu(cpu, mask);
>   			continue;
> @@ -284,7 +307,12 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>   	if (!sink_ops(sink)->alloc_buffer || !sink_ops(sink)->free_buffer)
>   		goto err;
>   
> -	/* Allocate the sink buffer for this session */
> +	/*
> +	 * Allocate the sink buffer for this session. All the sinks
> +	 * where this event can be scheduled are ensured to be of the
> +	 * same type. Thus the same sink configuration is used by the
> +	 * sinks.
> +	 */
>   	event_data->snk_config =
>   			sink_ops(sink)->alloc_buffer(sink, event, pages,
>   						     nr_pages, overwrite);
>