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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ9a7ViUOZEyvqe-KGc-UdhmkqXpPdSeBWjk-=u1tAjFKu5Y+A@mail.gmail.com>
Date: Tue, 1 Apr 2025 15:35:44 +0100
From: Mike Leach <mike.leach@...aro.org>
To: Suzuki K Poulose <suzuki.poulose@....com>
Cc: Leo Yan <leo.yan@....com>, James Clark <james.clark@...aro.org>, 
	Jonathan Corbet <corbet@....net>, Alexander Shishkin <alexander.shishkin@...ux.intel.com>, 
	coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/6] coresight: perf: Update buffer on AUX pause

On Tue, 1 Apr 2025 at 13:52, Suzuki K Poulose <suzuki.poulose@....com> wrote:
>
> On 11/03/2025 17:04, Leo Yan wrote:
> > Due to sinks like ETR and ETB don't support interrupt handling, the
> > hardware trace data might be lost for continuous running tasks.
> >
> > This commit takes advantage of the AUX pause for updating trace buffer
> > to mitigate the trace data losing issue.
> >
> > The per CPU sink has its own interrupt handling.  Thus, there will be a
> > race condition between the updating buffer in NMI and sink's interrupt
> > handler.  To avoid the race condition, this commit disallows updating
> > buffer on AUX pause for the per CPU sink.  Currently, this is only
> > applied for TRBE.
> >
> > Signed-off-by: Leo Yan <leo.yan@....com>
> > ---
> >   .../hwtracing/coresight/coresight-etm-perf.c  | 43 ++++++++++++++++++-
> >   1 file changed, 41 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > index 2dcf1809cb7f..f1551c08ecb2 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > @@ -574,14 +574,53 @@ static void etm_event_start(struct perf_event *event, int flags)
> >       return;
> >   }
> >
> > -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.
>
> Cheers
> Suzuki
>

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.


Regards

Mike

>
> > +     if (READ_ONCE(handle->event)) {
> > +             if (!size)
> > +                     return;
> > +
> > +             perf_aux_output_end(handle, size);
> > +             perf_aux_output_begin(handle, event);
> > +     } else {
> > +             WARN_ON_ONCE(size);
> > +     }
> >   }
> >
> >   static void etm_event_stop(struct perf_event *event, int mode)
> > @@ -595,7 +634,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
> >       struct coresight_path *path;
> >
> >       if (mode & PERF_EF_PAUSE)
> > -             return etm_event_pause(csdev, ctxt);
> > +             return etm_event_pause(event, csdev, ctxt);
> >
> >       /*
> >        * If we still have access to the event_data via handle,
>


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