Re: [PATCH v3 5/6] coresight: perf: Update buffer on AUX pause

[Leo Yan]

On Tue, Apr 01, 2025 at 03:35:44PM +0100, Mike Leach wrote:

[...]

> > > -static void etm_event_pause(struct coresight_device *csdev,
> > > +static void etm_event_pause(struct perf_event *event,
> > > +                         struct coresight_device *csdev,
> > >                           struct etm_ctxt *ctxt)
> > >   {
> > > +     int cpu = smp_processor_id();
> > > +     struct coresight_device *sink;
> > > +     struct perf_output_handle *handle = &ctxt->handle;
> > > +     struct coresight_path *path;
> > > +     unsigned long size;
> > > +
> > >       if (!ctxt->event_data)
> > >               return;
> > >
> > >       /* Stop tracer */
> > >       coresight_pause_source(csdev);
> > > +
> > > +     path = etm_event_cpu_path(ctxt->event_data, cpu);
> > > +     sink = coresight_get_sink(path);
> > > +     if (WARN_ON_ONCE(!sink))
> > > +             return;
> > > +
> > > +     /*
> > > +      * The per CPU sink has own interrupt handling, it might have
> > > +      * race condition with updating buffer on AUX trace pause if
> > > +      * it is invoked from NMI.  To avoid the race condition,
> > > +      * disallows updating buffer for the per CPU sink case.
> > > +      */
> > > +     if (coresight_is_percpu_sink(sink))
> > > +             return;
> > > +
> > > +     if (WARN_ON_ONCE(handle->event != event))
> > > +             return;
> > > +
> > > +     if (!sink_ops(sink)->update_buffer)
> > > +             return;
> > > +
> > > +     size = sink_ops(sink)->update_buffer(sink, handle,
> > > +                                          ctxt->event_data->snk_config);
> >
> > I believe we keep the sink disabled/stopped in update_buffer. We need to
> > turn it back ON after the "buffer update". May be we could use a flag
> > to update_buffer() to indicate this is "pause" triggered update.

Thanks for pointing out, Suzuki.

I will fix it in next spin.

> The sink is stopped to read data, but also important is the
> enable/disable refcount. The only time that "update_buffer" will read
> is if the sink has a refcount == 1. These are ultimately controlled by
> enable/disable path - which will not occur during pause/resume
> operations.

Hi Mike,

My understanding is: if a sink is shared by multiple CPUs, e.g. a Perf
session with CPU mode or system wide mode, then we cannot arbitrarily
disable sink and update buffer, otherwise, it might cause hardware
lockup issue.

For per-thread mode, the refcount == 1, we can take chance to update
buffer, right?

Thanks,
Leo