linux-kernel - Re: [PATCH 1/2] perf trace: do not lose last events in a race

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: <CAH0uvohodYgZ9hL=fRauk_85+sNjAVvjdB1UxKouMLLURYTbuQ@mail.gmail.com>
Date: Thu, 7 Nov 2024 13:46:19 -0800
From: Howard Chu <howardchu95@...il.com>
To: Benjamin Peterson <benjamin@...flow.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Ian Rogers <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>, 
	"Liang, Kan" <kan.liang@...ux.intel.com>, 
	"open list:PERFORMANCE EVENTS SUBSYSTEM" <linux-perf-users@...r.kernel.org>, 
	"open list:PERFORMANCE EVENTS SUBSYSTEM" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] perf trace: do not lose last events in a race

Hello Benjamin,

Without your patch (no output):
perf $ ./perf trace -e syscalls:sys_enter_exit_group true
perf $

With your patch:

perf $ ./perf trace -e syscalls:sys_enter_exit_group true
     0.000 true/1530009 syscalls:sys_enter_exit_group()

The result looks good.

Tested-by: Howard Chu <howardchu95@...il.com>

Thanks,
Howard

On Wed, Nov 6, 2024 at 3:45 PM Benjamin Peterson <benjamin@...flow.com> wrote:
>
> If a perf trace event selector specifies a maximum number of events to output
> (i.e., "/nr=N/" syntax), the event printing handler, trace__event_handler,
> disables the event selector after the maximum number events are
> printed. Furthermore, trace__event_handler checked if the event selector was
> disabled before doing any work. This avoided exceeding the maximum number of
> events to print if more events were in the buffer before the selector was
> disabled. However, the event selector can be disabled for reasons other than
> exceeding the maximum number of events. In particular, when the traced
> subprocess exits, the main loop disables all event selectors. This meant the
> last events of a traced subprocess might be lost to the printing handler's
> short-circuiting logic.
>
> This nondeterministic problem could be seen by running the following many times:
>
>   $ perf trace -e syscalls:sys_enter_exit_group true
>
> trace__event_handler should simply check for exceeding the maximum number of
> events to print rather than the state of the event selector.
>
> Fixes: a9c5e6c1e9bff ("perf trace: Introduce per-event maximum number of events property")
> Signed-off-by: Benjamin Peterson <benjamin@...flow.com>
> ---
>  tools/perf/builtin-trace.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index d3f11b90d025..f6179b13b8b4 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -3096,13 +3096,8 @@ static int trace__event_handler(struct trace *trace, struct evsel *evsel,
>  {
>         struct thread *thread;
>         int callchain_ret = 0;
> -       /*
> -        * Check if we called perf_evsel__disable(evsel) due to, for instance,
> -        * this event's max_events having been hit and this is an entry coming
> -        * from the ring buffer that we should discard, since the max events
> -        * have already been considered/printed.
> -        */
> -       if (evsel->disabled)
> +
> +       if (evsel->nr_events_printed >= evsel->max_events)
>                 return 0;
>
>         thread = machine__findnew_thread(trace->host, sample->pid, sample->tid);
> --
> 2.39.5
>
>