Re: [PATCH 06/10] coresight: perf: traceid: Add perf notifiers for trace ID

[Mathieu Poirier <mathieu.poirier@linaro.org>]

Good morning,

On Wed, Apr 06, 2022 at 08:38:08PM +0100, Mike Leach wrote:
> Hi Mathieu,
> 
> On Wed, 6 Apr 2022 at 18:11, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> >
> > On Tue, Mar 08, 2022 at 08:49:56PM +0000, Mike Leach wrote:
> > > Adds in notifier calls to the trace ID allocator that perf
> > > events are starting and stopping.
> > >
> > > This ensures that Trace IDs associated with CPUs remain the same
> > > throughout the perf session, and are only release when all perf
> > > sessions are complete.
> > >
> > > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > > ---
> > >  drivers/hwtracing/coresight/coresight-etm-perf.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > index c039b6ae206f..008f9dac429d 100644
> > > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > > @@ -22,6 +22,7 @@
> > >  #include "coresight-etm-perf.h"
> > >  #include "coresight-priv.h"
> > >  #include "coresight-syscfg.h"
> > > +#include "coresight-trace-id.h"
> > >
> > >  static struct pmu etm_pmu;
> > >  static bool etm_perf_up;
> > > @@ -223,11 +224,21 @@ static void free_event_data(struct work_struct *work)
> > >               struct list_head **ppath;
> > >
> > >               ppath = etm_event_cpu_path_ptr(event_data, cpu);
> > > -             if (!(IS_ERR_OR_NULL(*ppath)))
> > > +             if (!(IS_ERR_OR_NULL(*ppath))) {
> > >                       coresight_release_path(*ppath);
> > > +                     /*
> > > +                      * perf may have read a trace id for a cpu, but never actually
> > > +                      * executed code on that cpu - which means the trace id would
> > > +                      * not release on disable. Re-release here to be sure.
> > > +                      */
> > > +                     coresight_trace_id_put_cpu_id(cpu, coresight_get_trace_id_map());
> >
> > A CPU gets a traceID in event_etm_start() when the event is installed for
> > running.  Do you see a scenario where etm_free_aux() is called without
> > previously calling event_etm_stop()?
> >
> 
> To ensure that IDs are obtained in a timely manner, they assign on
> read. Therefore when cs_etm.c::cs_etm_info_fill() is called,
> potentially the ID will be read for all CPUs - and dumped into the
> AUXINFO data at the top of the perf.data file.

Right, I realised that when I got to the perf tools part.  If we end up keeping
the current approach it would be nice to see this explanation in the comment.
Otherwise it will be very difficult for anyone new to the project to understand
what is going on.

> However, a --per-thread execution may actually only start the event on
> a subset of cpus, hence we ensure that all cpus are released.
> This was proven during testing when I ran both --per-thread and cpu
> wide tests with logging monitoring the ID assignments.
> 
> I have programmed this deliberately defensively - on the basis that
> the timings and orderings of the code/callbacks in todays perf may not
> necessarily be the same in tomorrows. perf.
> 
> In future we may be able to use Suzuki's idea of embedding the ID into
> an alternative packet in the perf.data file. but I think that should
> be a separate change as it affects decode in a big way.
>

I really like Suzuki's idea of using a PERF_RECORD_AUX_OUTPUT_HW_ID to convey
the traceID to user space for perf sessions.  At the very least it is worth
prototyping.

I generally prefer an incremental approach but in this case it might be better
to move forward the right way, right away.  We would also avoid having to
maintain the old way, the intermediate way and the new way.

Thanks,
Mathieu

> Regards
> 
> Mike
> 
> 
> > > +             }
> > >               *ppath = NULL;
> > >       }
> > >
> > > +     /* mark perf event as done for trace id allocator */
> > > +     coresight_trace_id_perf_stop();
> > > +
> > >       free_percpu(event_data->path);
> > >       kfree(event_data);
> > >  }
> > > @@ -314,6 +325,9 @@ static void *etm_setup_aux(struct perf_event *event, void **pages,
> > >               sink = user_sink = coresight_get_sink_by_id(id);
> > >       }
> > >
> > > +     /* tell the trace ID allocator that a perf event is starting up */
> > > +     coresight_trace_id_perf_start();
> > > +
> > >       /* check if user wants a coresight configuration selected */
> > >       cfg_hash = (u32)((event->attr.config2 & GENMASK_ULL(63, 32)) >> 32);
> > >       if (cfg_hash) {
> > > --
> > > 2.17.1
> > >
> 
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK