From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id ADD1EC43387 for ; Wed, 19 Dec 2018 09:40:50 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 786E121850 for ; Wed, 19 Dec 2018 09:40:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728680AbeLSJkt (ORCPT ); Wed, 19 Dec 2018 04:40:49 -0500 Received: from foss.arm.com ([217.140.101.70]:59668 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728649AbeLSJks (ORCPT ); Wed, 19 Dec 2018 04:40:48 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 53048A78; Wed, 19 Dec 2018 01:40:48 -0800 (PST) Received: from [10.1.196.93] (en101.cambridge.arm.com [10.1.196.93]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 35E793F575; Wed, 19 Dec 2018 01:40:45 -0800 (PST) Subject: Re: [RESEND PATCH v5 4/6] coresight: Use PMU driver configuration for sink selection To: Mathieu Poirier Cc: Arnaldo Carvalho de Melo , Peter Zijlstra , Greg KH , Ingo Molnar , Thomas Gleixner , Alexander Shishkin , schwidefsky@de.ibm.com, heiko.carstens@de.ibm.com, Will Deacon , Mark Rutland , Jiri Olsa , Namhyung Kim , Adrian Hunter , ast@kernel.org, "H. Peter Anvin" , suzuki.poulosi@arm.com, linux-s390@vger.kernel.org, Linux Kernel Mailing List , linux-arm-kernel References: <1545067306-31687-1-git-send-email-mathieu.poirier@linaro.org> <1545067306-31687-5-git-send-email-mathieu.poirier@linaro.org> <48afc315-d4ed-8779-a808-757fa4203bb7@arm.com> From: Suzuki K Poulose Message-ID: Date: Wed, 19 Dec 2018 09:40:43 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 18/12/2018 17:34, Mathieu Poirier wrote: > Good day Suzuki, > > On Tue, 18 Dec 2018 at 07:14, Suzuki K Poulose wrote: >> >> Hi Mathieu, >> >> On 17/12/2018 17:21, Mathieu Poirier wrote: >>> This patch uses the PMU driver configuration held in event::hw::drv_config >>> to select a sink for each event that is created (the old sysFS way of >>> working is kept around for backward compatibility). >>> >>> By proceeding in this way a sink can be used by multiple sessions >>> without having to play games with entries in sysFS. >>> >>> Signed-off-by: Mathieu Poirier >>> --- >>> drivers/hwtracing/coresight/coresight-etm-perf.c | 74 ++++++++++++++++++++---- >>> 1 file changed, 62 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c >>> index f21eb28b6782..a7e1fdef07f2 100644 >>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c >>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c >>> @@ -4,6 +4,7 @@ >>> * Author: Mathieu Poirier >>> */ >>> >>> +#include >>> #include >>> #include >>> #include >>> @@ -11,6 +12,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> #include >>> #include >>> @@ -177,6 +179,26 @@ static void etm_free_aux(void *data) >>> schedule_work(&event_data->work); >>> } >>> >>> +static struct coresight_device *etm_drv_config_sync(struct perf_event *event) >> >> minor nit: The name doesn't quite imply what we do here. Did you mean >> s/sync/sink ? >> > > I chose "sync" with "synchronisation" in mind. I tried to keep things > generic since we could potentially use the same interface to convey > complex PMU configuration. Arguably we could go with "sink" for now > and change it to "sync" in the future - I'm not strongly opinionated > on that part. Ok. I thought we were trying to grab the sink information from the event drv_config, hence something that implies that would be slightly more reader friendly. Again, I am not too keen on it. > >>> +{ >>> + struct coresight_device *sink = NULL; >>> + struct pmu_drv_config *drv_config = perf_event_get_drv_config(event); >>> + >>> + /* >>> + * Make sure we don't race with perf_drv_config_replace() in >>> + * kernel/events/core.c. >>> + */ >>> + raw_spin_lock(&drv_config->lock); >>> + >>> + /* Copy what we got from user space if applicable. */ >>> + if (drv_config->config) >>> + sink = drv_config->config; >>> + >>> + raw_spin_unlock(&drv_config->lock); >>> + >>> + return sink; >>> +} >>> + >>> static void *etm_setup_aux(struct perf_event *event, void **pages, >>> int nr_pages, bool overwrite) >>> { >>> @@ -190,18 +212,11 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, >>> return NULL; >>> INIT_WORK(&event_data->work, free_event_data); >>> >>> - /* >>> - * In theory nothing prevent tracers in a trace session from being >>> - * associated with different sinks, nor having a sink per tracer. But >>> - * until we have HW with this kind of topology we need to assume tracers >>> - * in a trace session are using the same sink. Therefore go through >>> - * the coresight bus and pick the first enabled sink. >>> - * >>> - * When operated from sysFS users are responsible to enable the sink >>> - * while from perf, the perf tools will do it based on the choice made >>> - * on the cmd line. As such the "enable_sink" flag in sysFS is reset. >>> - */ >>> - sink = coresight_get_enabled_sink(true); >>> + /* First get the sink config from user space. */ >>> + sink = etm_drv_config_sync(event); >>> + if (!sink) >>> + sink = coresight_get_enabled_sink(true); >>> + >>> if (!sink || !sink_ops(sink)->alloc_buffer) >>> goto err; >>> >>> @@ -454,6 +469,40 @@ static void etm_addr_filters_sync(struct perf_event *event) >>> filters->nr_filters = i; >>> } >>> >>> +static int etm_drv_config_find_sink(struct device *dev, void *data) >>> +{ >>> + struct amba_device *adev = to_amba_device(dev->parent); >>> + struct resource *res = &adev->res; >>> + u64 value = *((u64 *)data); >>> + >>> + /* >>> + * The HW mapping of a component is unique. If the value we've been >>> + * given matches the component's start address, then we must have found >>> + * the device we are looking for. >>> + */ >> >> To be frank, I don't quite like the idea of passing the base address of the >> component as the key to locate a device, (even though that is unique and readily >> available). I would rather prefer a programmable way to map the keys to the >> "sink" devices, which works platform agnostic (e.g, ACPI support, where the base >> address is not obvious from the name). Also if we decide to use a platform >> agnostic naming scheme, it becomes even more complex. > > This mechanism doesn't rely on the naming scheme - it exploits the > "resource" interface exported for each amba device [1]. As such > whether the component is discovered using ACPI or DT, we end up on the > same amba bus and using the same interface. > > [1]. https://elixir.bootlin.com/linux/latest/source/drivers/amba/bus.c#L128 Ok. The only problem with this approach would be if the devices doesn't appear on the AMBA bus (btw, which is not true for the existing IPs). > >> >> We could assign a static "id/key" exported either via the device sysfs dir or >> the "pmu" dir. I prefer the latter. > > Not sure what you mean by "pmu" directory - would you mind expanding > on that? Using sysfs would be quite easy but I am reluctant to create > a new id/key mechanism and introduce another entry when we have the > component address that is unique and already available in the amba > directory structure. We could add another directory under : /sys/bus/event_source/devices// \_ events/ \_ format/ say : \_ drv_config/ Or \_ sinks/ and list the sinks, eg: # cd $sysfs_pmu_dir/sinks # cat ID_of_the_sink Btw, I am always inclined to using some bits off one of the "config" fields ("config1" or "config2") for the sink configuration. But I understand that you have explored that avenue and chose this approach as we have further configurations required for complex ETM settings. Cheers Suzuki