lkml.org 
[lkml]   [2016]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 03/10] coresight: etm-perf: configuring filters from perf core
On 21 July 2016 at 09:15, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> On 20 July 2016 at 10:07, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>> On 18/07/16 20:51, Mathieu Poirier wrote:
>>>
>>> This patch implements the required API needed to access
>>> and retrieve range and start/stop filters from the perf core.
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> ---
>>> drivers/hwtracing/coresight/coresight-etm-perf.c | 146
>>> ++++++++++++++++++++---
>>> drivers/hwtracing/coresight/coresight-etm-perf.h | 32 +++++
>>> 2 files changed, 162 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> index 78a1bc0013a2..fde7f42149c5 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> @@ -29,6 +29,7 @@
>>> #include <linux/workqueue.h>
>>>
>>> #include "coresight-priv.h"
>>> +#include "coresight-etm-perf.h"
>>>
>>> static struct pmu etm_pmu;
>>> static bool etm_perf_up;
>>> @@ -83,12 +84,44 @@ static const struct attribute_group
>>> *etm_pmu_attr_groups[] = {
>>>
>>> static void etm_event_read(struct perf_event *event) {}
>>>
>>> +static int etm_addr_filters_alloc(struct perf_event *event)
>>> +{
>>
>>
>> ...
>>
>>> + return 0;
>>> +}
>>> +
>>
>>
>>
>>> +
>>> static int etm_event_init(struct perf_event *event)
>>> {
>>> + int ret;
>>> +
>>> if (event->attr.type != etm_pmu.type)
>>> return -ENOENT;
>>>
>>> - return 0;
>>> + ret = etm_addr_filters_alloc(event);
>>
>>
>>
>>> }
>>>
>>> static void free_event_data(struct work_struct *work)
>>> @@ -456,6 +489,85 @@ static void etm_free_drv_configs(struct perf_event
>>> *event)
>>> }
>>> }
>>>
>>> +static int etm_addr_filters_validate(struct list_head *filters)
>>> +{
>>
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void etm_addr_filters_sync(struct perf_event *event)
>>> +{
>>> + struct perf_addr_filters_head *head =
>>> perf_event_addr_filters(event);
>>> + unsigned long start, stop, *offs = event->addr_filters_offs;
>>> + struct etm_filters *filters = event->hw.addr_filters;
>>> + struct perf_addr_filter *filter;
>>> + int i = 0;
>>
>>
>> Is it possible to delay the etm_addr_filters_alloc() until this point ?
>> I understand that this function cannot report back failures if we fail
>> to allocate memory. Or may be do a lazy allocation from
>> addr_filters_validate(),
>> when we get the first filter added.
>
> Humm... You want to avoid allocating memory that may never be used if
> filters aren't specified. Ok, let's do the allocation in
> addr_filters_validate().
>

On second thought we don't have access to the perf event in
addr_filters_validate() so the previous strategy won't work. And as
you said etm_addr_filters_sync() doesn't return an error code so that
won't work either. As such I will keep the current code as is.

Mathieu

>>
>> Of course this could be done as a follow up patch to improve things once
>> we get the initial framework in.
>>
>>
>>
>>> +
>>> + list_for_each_entry(filter, &head->list, entry) {
>>> + start = filter->offset + offs[i];
>>> + stop = start + filter->size;
>>> +
>>> + if (filter->range == 1) {
>>> + filters->filter[i].start_addr = start;
>>> + filters->filter[i].stop_addr = stop;
>>> + filters->filter[i].type = ETM_ADDR_TYPE_RANGE;
>>> + } else {
>>> + if (filter->filter == 1) {
>>> + filters->filter[i].start_addr = start;
>>> + filters->filter[i].type =
>>> ETM_ADDR_TYPE_START;
>>> + } else {
>>> + filters->filter[i].stop_addr = stop;
>>> + filters->filter[i].type =
>>> ETM_ADDR_TYPE_STOP;
>>> + }
>>> + }
>>> + i++;
>>> + }
>>> +
>>> + filters->nr_filters = i;
>>> +/**
>>> + * struct etm_filters - set of filters for a session
>>> + * @etm_filter: All the filters for this session.
>>> + * @nr_filters: Number of filters
>>> + * @ssstatus: Status of the start/stop logic.
>>> + */
>>> +struct etm_filters {
>>> + struct etm_filter filter[ETM_ADDR_CMP_MAX];
>>
>>
>> nit: having the variable renamed to etm_filter will make the code a bit more
>> readable
>> where we populate/validate the filters.
>
> Very well.
>
> Thanks,
> Mathieu
>
>>
>> Otherwise looks good
>>
>> Suzuki

\
 
 \ /
  Last update: 2016-07-21 20:01    [W:0.052 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site