Date:   Wed, 1 Feb 2023 22:00:51 -0300
From:   Arnaldo Carvalho de Melo <acme@kernel.org>
To:     Adrian Hunter <adrian.hunter@intel.com>
Cc:     Krister Johansen <kjlx@templeofstupid.com>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Ian Rogers <irogers@google.com>,
        Michael Petlan <mpetlan@redhat.com>,
        David Reaver <me@davidreaver.com>
Subject: Re: [PATCH v2] perf/util: Symbol lookup with kcore can fail if
 multiple segments match stext
Message-ID: <Y9sLQ1Iy5LJAaJ8L@kernel.org>
References: <20230124223531.GB1962@templeofstupid.com>
 <20230125183418.GD1963@templeofstupid.com>
 <7b5f872a-f52e-7129-e956-90e97ec7d911@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7b5f872a-f52e-7129-e956-90e97ec7d911@intel.com>
Precedence: bulk

Em Mon, Jan 30, 2023 at 03:29:54PM +0200, Adrian Hunter escreveu:
> On 25/01/23 20:34, Krister Johansen wrote:
> > This problem was encountered on an arm64 system with a lot of memory.
> > Without kernel debug symbols installed, and with both kcore and kallsyms
> > available, perf managed to get confused and returned "unknown" for all
> > of the kernel symbols that it tried to look up.
> > 
> > On this system, stext fell within the vmalloc segment.  The kcore symbol
> > matching code tries to find the first segment that contains stext and
> > uses that to replace the segment generated from just the kallsyms
> > information.  In this case, however, there were two: a very large
> > vmalloc segment, and the text segment.  This caused perf to get confused
> > because multiple overlapping segments were inserted into the RB tree
> > that holds the discovered segments.  However, that alone wasn't
> > sufficient to cause the problem. Even when we could find the segment,
> > the offsets were adjusted in such a way that the newly generated symbols
> > didn't line up with the instruction addresses in the trace.  The most
> > obvious solution would be to consult which segment type is text from
> > kcore, but this information is not exposed to users.
> > 
> > Instead, select the smallest matching segment that contains stext
> > instead of the first matching segment.  This allows us to match the text
> > segment instead of vmalloc, if one is contained within the other.
> > 
> > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> 
> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

Thanks, applied.

- Arnaldo

 
> > ---
> > v2:
> >   - Correct whitespace, add comments, and fix-up subject. (Feedback from Adrian
> >     Hunter)
> > ---
> >  tools/perf/util/symbol.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index a3a165ae933a..98014f937568 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1368,10 +1368,23 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
> >  
> >  	/* Find the kernel map using the '_stext' symbol */
> >  	if (!kallsyms__get_function_start(kallsyms_filename, "_stext", &stext)) {
> > +		u64 replacement_size = 0;
> > +
> >  		list_for_each_entry(new_map, &md.maps, node) {
> > -			if (stext >= new_map->start && stext < new_map->end) {
> > +			u64 new_size = new_map->end - new_map->start;
> > +
> > +			if (!(stext >= new_map->start && stext < new_map->end))
> > +				continue;
> > +
> > +			/*
> > +			 * On some architectures, ARM64 for example, the kernel
> > +			 * text can get allocated inside of the vmalloc segment.
> > +			 * Select the smallest matching segment, in case stext
> > +			 * falls within more than one in the list.
> > +			 */
> > +			if (!replacement_map || new_size < replacement_size) {
> >  				replacement_map = new_map;
> > -				break;
> > +				replacement_size = new_size;
> >  			}
> >  		}
> >  	}
> 

-- 

- Arnaldo