Messages in this thread Patch in this message | | | Date | Wed, 8 May 2024 15:19:54 +0100 | Subject | Re: [PATCH 3/4] perf symbols: Update kcore map before merging in remaining symbols | From | Leo Yan <> |
| |
On 5/8/2024 10:14 AM, James Clark wrote:
[...]
>> Looks like you and Leo are working on the same problem. >> >> https://lore.kernel.org/r/20240505202805.583253-1-leo.yan@arm.com/ >> > > Oops I should have checked the list. It looks like we can still take his > fix as well though, with an updated comment.
Sorry for duplicate work. I will resend my patch separately with refined comment, as suggested by Adrian.
[...]
>>> @@ -1289,7 +1289,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map, >>> { >>> struct maps *kmaps = map__kmaps(map); >>> struct kcore_mapfn_data md; >>> - struct map *replacement_map = NULL; >>> + struct map *map_ref, *replacement_map = NULL; >>> struct machine *machine; >>> bool is_64_bit; >>> int err, fd; >>> @@ -1367,6 +1367,24 @@ 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;
As the 'replacement' map is mainly used to adjust the kernel's sections between '_stext' and '_end', some arches might don't share the same issue with Arm64. So it is a bit redundant for assignment 'replacement_map' if it is NULL, we can consider to remove the above two lines.
>>> >>> + /* >>> + * Update addresses of vmlinux map. Re-insert it to ensure maps are >>> + * correctly ordered. Do this before using maps__merge_in() for the >>> + * remaining maps so vmlinux gets split if necessary. >>> + */ >>> + map_ref = map__get(map); >>> + maps__remove(kmaps, map_ref); >> >> A nitpick. It'd be natural to use 'map' instead of 'map_ref' >> (even if they are the same) since IIUC we want to remove >> the old 'map' and update 'map_ref' then add it back. >> > > Using map makes sense, I can update that. > >>> + >>> + map__set_start(map_ref, map__start(replacement_map)); >>> + map__set_end(map_ref, map__end(replacement_map)); >>> + map__set_pgoff(map_ref, map__pgoff(replacement_map)); >>> + map__set_mapping_type(map_ref, map__mapping_type(replacement_map)); >> >> So here, replacement_map should not be NULL right? >> > > Yes it shouldn't be. It would only be NULL if md.maps is empty, but > there's already an exit condition for that above. > > Some of the other code also assumes node->map is always set, so it can't > be NULL that way either.
Thus, we can consider to check condition for 'replacement' map is NULL or not.
if (replacement_map) { list_del_init(&new_node->node);
map_ref = map__get(map); maps__remove(kmaps, map_ref); ... map__put(new_map); if (err) goto out_err; free(new_node); }
[...]
>>> @@ -1374,24 +1392,8 @@ static int dso__load_kcore(struct dso *dso, struct map *map, >>> >>> list_del_init(&new_node->node); >>> >>> - if (RC_CHK_EQUAL(new_map, replacement_map)) { >>> - struct map *map_ref; >>> - >>> - /* Ensure maps are correctly ordered */ >>> - map_ref = map__get(map); >>> - maps__remove(kmaps, map_ref); >>> - >>> - map__set_start(map_ref, map__start(new_map)); >>> - map__set_end(map_ref, map__end(new_map)); >>> - map__set_pgoff(map_ref, map__pgoff(new_map)); >>> - map__set_mapping_type(map_ref, map__mapping_type(new_map)); >>> - >>> - err = maps__insert(kmaps, map_ref); >>> - map__put(map_ref); >>> - map__put(new_map); >>> - if (err) >>> - goto out_err; >>> - } else { >>> + /* skip if replacement_map, already inserted above */ >>> + if (!RC_CHK_EQUAL(new_map, replacement_map)) {
With above change, we don't need check 'replacement_map' at here.
Just extend a bit for considering a more clean fixing, we need to sort all ranges in 'md.maps', this would be benefit for two things:
- We can fix up any map regions, not only limit to the 'replacement_map'. With sorting maps in 'md.maps', we can totally remove the code for 'replacement_map'. - We can report the potential issue caused by overlapping in the first place rather than the assert log in check_invariants(). This is easier for later debugging.
But current patch is good enough for me, I don't have strong opinion for this.
Thanks, Leo
>>> /* >>> * Merge kcore map into existing maps, >>> * and ensure that current maps (eBPF) >>> -- >>> 2.34.1 >>>
| |