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

[Mike Leach <mike.leach@linaro.org>]

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?

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
algorithm to ensure that only the best system memory type sinks are
found and eliminate cores attached to only buffer type sinks.

>
> We can defer all of this, until we get platforms which ned the support.
>
> Suzuki

Thanks for the review.

Mike


-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

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