Re: [PATCH v2] trace/trace_event_perf: remove duplicate samples on the first tracepoint event

[Namhyung Kim <namhyung@kernel.org>]

Hello,

On Tue, Oct 01, 2024 at 08:48:45AM +0000, Yeo Reum Yun wrote:
> Gentle ping in case of forgotten.
> 
> ________________________________________
> From: Steven Rostedt <rostedt@goodmis.org>
> Sent: 26 September 2024 13:51
> To: Yeo Reum Yun
> Cc: mhiramat@kernel.org; mathieu.desnoyers@efficios.com; a.p.zijlstra@chello.nl; mingo@elte.hu; Mark Rutland; james.clark@linaro.org; nd; linux-kernel@vger.kernel.org; linux-trace-kernel@vger.kernel.org; Peter Zijlstra; Frederic Weisbecker; Arnaldo Carvalho de Melo; Namhyung Kim
> Subject: Re: [PATCH v2] trace/trace_event_perf: remove duplicate samples on the first tracepoint event
> 
> Can one of the perf folks give me an Ack, and I'll take this through my tree.

I'm not sure if we want to support freq mode or period other than 1 for
tracepoint events.  But it looks broken apparently, so

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> 
> On Fri, 13 Sep 2024 03:13:47 +0100
> Levi Yun <yeoreum.yun@arm.com> wrote:
> 
> > When a tracepoint event is created with attr.freq = 1,
> > 'hwc->period_left' is not initialized correctly. As a result,
> > in the perf_swevent_overflow() function, when the first time the event occurs,
> > it calculates the event overflow and the perf_swevent_set_period() returns 3,
> > this leads to the event are recorded for three duplicate times.
> >
> > Step to reproduce:
> >     1. Enable the tracepoint event & starting tracing
> >          $ echo 1 > /sys/kernel/tracing/events/module/module_free
> >          $ echo 1 > /sys/kernel/tracing/tracing_on
> >
> >     2. Record with perf
> >          $ perf record -a --strict-freq -F 1 -e "module:module_free"
> >
> >     3. Trigger module_free event.
> >          $ modprobe -i sunrpc
> >          $ modprobe -r sunrpc
> >
> > Result:
> >      - Trace pipe result:
> >          $ cat trace_pipe
> >          modprobe-174509  [003] .....  6504.868896: module_free: sunrpc
> >
> >      - perf sample:
> >          modprobe  174509 [003]  6504.868980: module:module_free: sunrpc
> >          modprobe  174509 [003]  6504.868980: module:module_free: sunrpc
> >          modprobe  174509 [003]  6504.868980: module:module_free: sunrpc
> >
> > By setting period_left via perf_swevent_set_period() as other sw_event did,
> > This problem could be solved.
> >
> > After patch:
> >      - Trace pipe result:
> >          $ cat trace_pipe
> >          modprobe 1153096 [068] 613468.867774: module:module_free: xfs
> >
> >      - perf sample
> >          modprobe 1153096 [068] 613468.867794: module:module_free: xfs
> >
> > Fixes: bd2b5b12849a ("perf_counter: More aggressive frequency adjustment")
> > Signed-off-by: Levi Yun <yeoreum.yun@arm.com>
> > ---
> > Changes in v2:
> >   - Fix build error.
> > ---
> >  kernel/trace/trace_event_perf.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> > index 05e791241812..3ff9caa4a71b 100644
> > --- a/kernel/trace/trace_event_perf.c
> > +++ b/kernel/trace/trace_event_perf.c
> > @@ -352,10 +352,16 @@ void perf_uprobe_destroy(struct perf_event *p_event)
> >  int perf_trace_add(struct perf_event *p_event, int flags)
> >  {
> >       struct trace_event_call *tp_event = p_event->tp_event;
> > +     struct hw_perf_event *hwc = &p_event->hw;
> >
> >       if (!(flags & PERF_EF_START))
> >               p_event->hw.state = PERF_HES_STOPPED;
> >
> > +     if (is_sampling_event(p_event)) {
> > +             hwc->last_period = hwc->sample_period;
> > +             perf_swevent_set_period(p_event);
> > +     }
> > +
> >       /*
> >        * If TRACE_REG_PERF_ADD returns false; no custom action was performed
> >        * and we need to take the default action of enqueueing our event on
> > --
> > LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}
>