Received-SPF: pass (google.com: domain of linux-kernel+bounces-172739-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <479cfef5-daa8-4650-85c7-4a5764885562@intel.com>
Date: Wed, 8 May 2024 09:18:18 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] perf maps: Process kcore maps in order
To: Leo Yan <leo.yan@linux.dev>
Cc: Leo Yan <leo.yan@arm.com>, Arnaldo Carvalho de Melo <acme@kernel.org>,
 Ian Rogers <irogers@google.com>, Namhyung Kim <namhyung@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>, Athira Rajeev <atrajeev@linux.vnet.ibm.com>,
 James Clark <james.clark@arm.com>, linux-perf-users@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240505202805.583253-1-leo.yan@arm.com>
 <d47346fc-51b4-4af5-a014-0bd6f3b7bae0@intel.com>
 <20240507210151.GB1384@debian-dev>
Content-Language: en-US
From: Adrian Hunter <adrian.hunter@intel.com>
Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki,
 Business Identity Code: 0357606 - 4, Domiciled in Helsinki
In-Reply-To: <20240507210151.GB1384@debian-dev>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 8/05/24 00:01, Leo Yan wrote:
> Hi Adrian,
> 
> On Mon, May 06, 2024 at 08:43:01AM +0300, Adrian Hunter wrote:
> 
> [...]
> 
>>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>>> index 9ebdb8e13c0b..e15d70845488 100644
>>> --- a/tools/perf/util/symbol.c
>>> +++ b/tools/perf/util/symbol.c
>>> @@ -1266,7 +1266,24 @@ static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data)
>>>  	map__set_end(list_node->map, map__start(list_node->map) + len);
>>>  	map__set_pgoff(list_node->map, pgoff);
>>>  
>>> -	list_add(&list_node->node, &md->maps);
>>> +	/*
>>> +	 * Kcore maps are ordered with:
>>> +	 *   [_text.._end): Kernel text section
>>> +	 *   [VMALLOC_START..VMALLOC_END): vmalloc
>>> +	 *   ...
>>> +	 *
>>> +	 * On Arm64, the '_text' and 'VMALLOC_START' are the same values
>>> +	 * but VMALLOC_END (~124TiB) is much bigger then the text end
>>> +	 * address. So '_text' region is the subset of the vmalloc region.
>>> +	 *
>>> +	 * Afterwards, when dso__load_kcore() adjusts kernel maps, we must
>>> +	 * process the kernel text size prior to handling vmalloc region.
>>> +	 * This can avoid to using any inaccurate kernel text size when
>>> +	 * extending maps with vmalloc region. For this reason, here it
>>> +	 * always adds kcore maps to the tail of list to make sure the
>>> +	 * sequential handling is in order.
>>> +	 */
>>> +	list_add_tail(&list_node->node, &md->maps);
>>
>> This seems reasonable, but I wonder if it might be robust
>> and future proof to also process the main map first
>> e.g. totally untested:
> 
> Makes sense for me, I verified your proposal with a minor improvment,
> please see the comment below.
> 
>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>> index 9ebdb8e13c0b..63bce45a5abb 100644
>> --- a/tools/perf/util/symbol.c
>> +++ b/tools/perf/util/symbol.c
>> @@ -1365,16 +1365,15 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>>  	if (!replacement_map)
>>  		replacement_map = list_entry(md.maps.next, struct map_list_node, node)->map;
>>  
>> -	/* Add new maps */
>> +	/* Add replacement_map */
>>  	while (!list_empty(&md.maps)) {
> 
> For the replacement map, as we have located it in the list, here we
> don't need to iterate the whole kcore map list anymore. We can
> directly use the replacement map to update the passed map:
> 
>         /* Update replacement_map */
>         if (replacement_map) {
>                 struct map *map_ref;
> 
>                 list_del_init(&replacement_node->node);
>                 map__set_start(map, map__start(replacement_map));
>                 map__set_end(map, map__end(replacement_map));
>                 map__set_pgoff(map, map__pgoff(replacement_map));
>                 map__set_mapping_type(map, map__mapping_type(replacement_map));
>                 /* Ensure maps are correctly ordered */
>                 map_ref = map__get(map);
>                 maps__remove(kmaps, map_ref);
>                 err = maps__insert(kmaps, map_ref);
>                 map__put(map_ref);
>                 map__put(replacement_map);
>                 if (err)
>                         goto out_err;
>                 free(replacement_node);
>         }
> 
> I also uploaded the verified change to https://termbin.com/rrfo.
> 
> Please let me know if you would like to send a patch for this, or you
> want me to spin a new version. Either is fine for me.

James has a patch that does this also and looks good:

https://lore.kernel.org/linux-perf-users/CAM9d7cjYvMndUmSuwnE1ETwnu_6WrxQ4UzsNHHvo4SVR250L7A@mail.gmail.com/T/#md3d61e4182fc5bc3aee917db9af23a39b617b8ea

However, the "list_add_tail" change still seems worth doing
because it is more logical to process in order rather than
reverse order.  Probably just need to adjust the comment and
commit message.