From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [RESEND PATCH v5 4/6] coresight: Use PMU driver configuration for sink selection 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 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org List-Archive: To: Mathieu Poirier Cc: Mark Rutland , linux-s390@vger.kernel.org, Peter Zijlstra , Greg KH , heiko.carstens@de.ibm.com, Adrian Hunter , Arnaldo Carvalho de Melo , ast@kernel.org, Alexander Shishkin , Ingo Molnar , Will Deacon , "H. Peter Anvin" , schwidefsky@de.ibm.com, Namhyung Kim , Thomas Gleixner , suzuki.poulosi@arm.com, Jiri Olsa , Linux Kernel Mailing List , linux-arm-kernel List-ID: 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel