Received-SPF: pass (google.com: domain of linux-kernel+bounces-166869-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 2 May 2024 16:38:14 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: Namhyung Kim <namhyung@kernel.org>,
	Kan Liang <kan.liang@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	linux-perf-users@vger.kernel.org
Subject: Re: [PATCH] perf maps: Remove check_invariants() from maps__lock()
Message-ID: <ZjPrpnX1XqyOHQku@x1>
References: <20240429225738.1491791-1-namhyung@kernel.org>
 <CAP-5=fWeFyihQNMEJ2BQpm6hmNBmPuwFk3rGgVh+jV0mKd7myg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAP-5=fWeFyihQNMEJ2BQpm6hmNBmPuwFk3rGgVh+jV0mKd7myg@mail.gmail.com>

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@kernel.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@google.com>

Thanks, applied to perf-tools-next,

- Arnaldo