Re: [PATCH v3 2/3] perf maps: Remove the kernel text map with maps__remove_maps()

[Adrian Hunter <adrian.hunter@intel.com>]

On 20/05/24 12:06, Leo Yan wrote:
> maps__remove_maps() removes all kernel maps except the text map and eBPF
> maps. Afterwards, the kernel text map is removed from the list and
> added again with updated information to the maps list.
> 
> This commit refactors maps__remove_maps() for deleting the 'map'
> parameter, resulting in the removal of all kernel maps from the list.
> Thus, the dso__load_kcore() function no longer needs to remove the kernel
> text map.
> 
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>  tools/perf/util/maps.c   | 4 ++--
>  tools/perf/util/maps.h   | 2 +-
>  tools/perf/util/symbol.c | 9 +++------
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index 16b39db594f4..4ddd0d50ac2c 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
> @@ -589,7 +589,7 @@ int maps__for_each_map(struct maps *maps, int (*cb)(struct map *map, void *data)
>  	return ret;
>  }
>  
> -void maps__remove_maps(struct maps *maps, bool (*cb)(struct map *map, void *data), void *data)
> +void maps__remove_maps(struct maps *maps, bool (*cb)(struct map *map))
>  {
>  	struct map **maps_by_address;
>  
> @@ -597,7 +597,7 @@ void maps__remove_maps(struct maps *maps, bool (*cb)(struct map *map, void *data
>  
>  	maps_by_address = maps__maps_by_address(maps);
>  	for (unsigned int i = 0; i < maps__nr_maps(maps);) {
> -		if (cb(maps_by_address[i], data))
> +		if (cb(maps_by_address[i]))
>  			__maps__remove(maps, maps_by_address[i]);
>  		else
>  			i++;
> diff --git a/tools/perf/util/maps.h b/tools/perf/util/maps.h
> index d9aa62ed968a..90a1ff8b39c5 100644
> --- a/tools/perf/util/maps.h
> +++ b/tools/perf/util/maps.h
> @@ -40,7 +40,7 @@ bool maps__equal(struct maps *a, struct maps *b);
>  /* Iterate over map calling cb for each entry. */
>  int maps__for_each_map(struct maps *maps, int (*cb)(struct map *map, void *data), void *data);
>  /* Iterate over map removing an entry if cb returns true. */
> -void maps__remove_maps(struct maps *maps, bool (*cb)(struct map *map, void *data), void *data);
> +void maps__remove_maps(struct maps *maps, bool (*cb)(struct map *map));
>  
>  struct machine *maps__machine(const struct maps *maps);
>  unsigned int maps__nr_maps(const struct maps *maps); /* Test only. */
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index c1513976ab6e..915435d55498 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1322,15 +1322,13 @@ static int kcore_mapfn(u64 start, u64 len, u64 pgoff, void *data)
>  	return 0;
>  }
>  
> -static bool remove_old_maps(struct map *map, void *data)
> +static bool remove_old_maps(struct map *map)
>  {
> -	const struct map *map_to_save = data;
> -
>  	/*
>  	 * We need to preserve eBPF maps even if they are covered by kcore,
>  	 * because we need to access eBPF dso for source data.
>  	 */
> -	return !RC_CHK_EQUAL(map, map_to_save) && !__map__is_bpf_prog(map);
> +	return !__map__is_bpf_prog(map);
>  }
>  
>  static int dso__load_kcore(struct dso *dso, struct map *map,
> @@ -1385,7 +1383,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>  	}
>  
>  	/* Remove old maps */
> -	maps__remove_maps(kmaps, remove_old_maps, map);
> +	maps__remove_maps(kmaps, remove_old_maps);
>  	machine->trampolines_mapped = false;
>  
>  	/* Find the kernel map using the '_stext' symbol */
> @@ -1422,7 +1420,6 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>  	 * remaining maps so vmlinux gets split if necessary.
>  	 */
>  	map_ref = map__get(map);

If the ref is needed, then it should be taken before
the call to maps__remove_maps().

> -	maps__remove(kmaps, map_ref);
>  
>  	map__set_start(map_ref, map__start(replacement_map));
>  	map__set_end(map_ref, map__end(replacement_map));