Re: [PATCH v4 4/5] coresight: etm: perf: Add default sink selection to etm perf

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 06/02/2020 02:12 PM, Mike Leach wrote:
> Hi Suzuki,
> 
> On Tue, 2 Jun 2020 at 12:40, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> On 05/26/2020 11:46 AM, Mike Leach wrote:
>>> Add default sink selection to the perf trace handling in the etm driver.
>>> Uses the select default sink infrastructure to select a sink for the perf
>>> session, if no other sink is specified.
>>>
>>> Signed-off-by: Mike Leach <mike.leach@linaro.org>
>>
>> This patch looks fine to me as such. But please see below for some
>> discussion on the future support for other configurations.
>>
>>
>>> ---
>>>    .../hwtracing/coresight/coresight-etm-perf.c    | 17 ++++++++++++++---
>>>    1 file changed, 14 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> index 84f1dcb69827..1a3169e69bb1 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
>>> @@ -226,9 +226,6 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>>>                sink = coresight_get_enabled_sink(true);
>>>        }
>>>
>>> -     if (!sink)
>>> -             goto err;
>>> -
>>>        mask = &event_data->mask;
>>>
>>>        /*
>>> @@ -253,6 +250,16 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
>>>                        continue;
>>>                }
>>>
>>> +             /*
>>> +              * 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.
>>> +              */
>>> +             if (!sink)
>>> +                     sink = coresight_find_default_sink(csdev);
>>> +
>>
>> While we are here, should we remove the "find enabled sink" if the csink
>> is not specified via config. ? That step is problematic, as the user may
>> not remember which sinks were enabled. Also, we can't hit that with
>> perf tool as it prevents any invocation without sink (until this change).
>>
>> So may be this is a good time to get rid of that ?
>>
> 
> You are correct - the  'sink = coresight_get_enabled_sink(true);' was
> dead code until this patch.
> However - if someone has set up their system using sysfs to enable
> sinks, then should we not respect that rather than assume they made a
> mistake?

If someone really wants to use a specific sink, then they could always
specify it via the config attribute and it will be honoured. We need not
carry along this non-intuitive hinting.

> 
> Thinking about N:M topologies mentioned below - one method of handling
> this is to enable relevant sinks and then let perf trace on any cores
> that might use them.
> 
>> Also, we may need to do special handling for cases where there multiple
>> sinks (ETRS) and the cpus in the event mask has different preferred
>> sink. We can defer it for now as we don't claim to support such
>> configurations yet.
> 
> Yes - the newer topologies will need some changes - beyond what we are
> handling here.
> However - especially for 1:1 -  the best way may be to always use the
> default sink - as specifying multiple sinks on the perf command line
> may be problematical.
> 
>> When we do, we could either :
>>
>> 1) Make sure the event is bound to a single CPU, in which case
>> the sink remains the same for the event.
>>
>> OR
>>
>> 2) All the different "preferred" sinks (ETRs selected by the ETM) have
>> the same capabilitiy. i.e, we can move around the "sink" specific
>> buffers and use them where we end up using.
> 
> If here by "capabilities" we are talking about buffer vs system memory
> type sinks then I agree. We may need in future to limit the search

Not necessarily. e.g, if we ever get two different types of system
memory sinks, (e.g, a global ETR and a dedicate "new" sink for a
cluster), we can't keep switching between the two sinks depending on how
they use the buffers. (i.e, direct buffer vs double copy)

Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel