Re: [PATCH v2 2/3] perf cs-etm: Remove callback cs_etm_find_snapshot()

[Mathieu Poirier]

On Thu, 1 Jul 2021 at 19:10, Leo Yan <leo.yan@xxxxxxxxxx> wrote:
>
> On Thu, Jul 01, 2021 at 01:25:14PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Jul 01, 2021 at 05:35:36PM +0800, Leo Yan escreveu:
> > > The callback cs_etm_find_snapshot() is invoked for snapshot mode, its
> > > main purpose is to find the correct AUX trace data and returns "head"
> > > and "old" (we can call "old" as "old head") to the caller, the caller
> > > __auxtrace_mmap__read() uses these two pointers to decide the AUX trace
> > > data size.
> > >
> > > This patch removes cs_etm_find_snapshot() with below reasons:
> > >
> > > - The first thing in cs_etm_find_snapshot() is to check if the head has
> > >   wrapped around, if it is not, directly bails out.  The checking is
> > >   pointless, this is because the "head" and "old" pointers both are
> > >   monotonical increasing so they never wrap around.
> > >
> > > - cs_etm_find_snapshot() adjusts the "head" and "old" pointers and
> > >   assumes the AUX ring buffer is fully filled with the hardware trace
> > >   data, so it always subtracts the difference "mm->len" from "head" to
> > >   get "old".  Let's imagine the snapshot is taken in very short
> > >   interval, the tracers only fill a small chunk of the trace data into
> > >   the AUX ring buffer, in this case, it's wrongly to copy the whole the
> > >   AUX ring buffer to perf file.
> > >
> > > - As the "head" and "old" pointers are monotonically increased, the
> > >   function __auxtrace_mmap__read() handles these two pointers properly.
> > >   It calculates the reminders for these two pointers, and the size is
> > >   clamped to be never more than "snapshot_size".  We can simply reply on
> > >   the function __auxtrace_mmap__read() to calculate the correct result
> > >   for data copying, it's not necessary to add Arm CoreSight specific
> > >   callback.
> >
> > Thanks, applied.
>
> Thanks a lot for picking up the patch, Arnaldo!
>
> Hi Mathieu, I supposed to get your review before merging; since
> Arnaldo moves quickly, if you want me to follow up anything relevant
> to this change, please let me know.  Thanks!

I was going to review this set next week.  But it is fine if Arnaldo
has picked it up since enough people have already looked at and tested
this code.

Thanks,
Mathieu

>
> Leo