Message-ID: <bb313a2d-b5fc-9329-5db6-4bf661d278bb@arm.com>
Date:   Wed, 19 Dec 2018 09:40:43 +0000
From:   Suzuki K Poulose <suzuki.poulose@....com>
To:     Mathieu Poirier <mathieu.poirier@...aro.org>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        schwidefsky@...ibm.com, heiko.carstens@...ibm.com,
        Will Deacon <will.deacon@....com>,
        Mark Rutland <mark.rutland@....com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>, ast@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, suzuki.poulosi@....com,
        linux-s390@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RESEND PATCH v5 4/6] coresight: Use PMU driver configuration for
 sink selection



On 18/12/2018 17:34, Mathieu Poirier wrote:
> Good day Suzuki,
> 
> On Tue, 18 Dec 2018 at 07:14, Suzuki K Poulose <suzuki.poulose@....com> 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 <mathieu.poirier@...aro.org>
>>> ---
>>>    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 <mathieu.poirier@...aro.org>
>>>     */
>>>
>>> +#include <linux/amba/bus.h>
>>>    #include <linux/coresight.h>
>>>    #include <linux/coresight-pmu.h>
>>>    #include <linux/cpumask.h>
>>> @@ -11,6 +12,7 @@
>>>    #include <linux/list.h>
>>>    #include <linux/mm.h>
>>>    #include <linux/init.h>
>>> +#include <linux/ioport.h>
>>>    #include <linux/perf_event.h>
>>>    #include <linux/percpu-defs.h>
>>>    #include <linux/slab.h>
>>> @@ -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/<PMU>/
				\_ events/
				\_ format/
say :
		\_ drv_config/
		Or
		\_ sinks/

and list the sinks, eg:
# cd $sysfs_pmu_dir/sinks
# cat <name_of_the_sink>
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

Powered by blists - more mailing lists