linux-kernel - Re: [PATCH] perf maps: Remove check_invariants() from maps_

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] [day] [month] [year] [list]

Message-ID: <ZjPrpnX1XqyOHQku@x1>
Date: Thu, 2 May 2024 16:38:14 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Namhyung Kim <namhyung@...nel.org>,
	Kan Liang <kan.liang@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
	linux-perf-users@...r.kernel.org
Subject: Re: [PATCH] perf maps: Remove check_invariants() from maps__lock()

On Mon, Apr 29, 2024 at 07:09:54PM -0700, Ian Rogers wrote:
> On Mon, Apr 29, 2024 at 3:57 PM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > I found that the debug build was a lot slowed down by the maps lock code
> > since it checks the invariants whenever it gets the pointer to the lock.
> > This means it checks twice the invariants before and after the access.
> >
> > Instead, let's move the checking code within the lock area but after any
> > modification and remove it from the read paths.  This would remove (more
> > than) the half of the maps lock overhead.
> >
> > The time for perf report with a huge data file (200k+ of MMAP2 events).
> >
> >   Non-debug     Before      After
> >   ---------   --------   --------
> >      2m 43s     6m 45s     4m 21s

Yeah, I had a debug build to check the size of 'struct hist_entry' with
pahole and noticed that invariant checking in 'perf top'.

> Thanks Namhyung, I think the change makes sense. There is an issue
> that a user of a map may mutate it in a way that breaks the maps
> sorting, symbol code would be a likely culprit of such a thing. A fix
> for such a breakage would be to just set the unsorted flag on maps.
> We'd be less likely to spot such an issue after this change, but it is
> simple enough to add the function call when needed. Another option
> would be to add another make flag like PARSER_DEBUG for these kind of,
> "be paranoid," type things. Anyway, let's try to fix those problems
> when they exist.
> 
> Reviewed-by: Ian Rogers <irogers@...gle.com>

Thanks, applied to perf-tools-next,

- Arnaldo