Received-SPF: pass (google.com: domain of linux-kernel+bounces-186135-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <8e5ab323-f893-4186-813b-e87394e0a9fd@intel.com>
Date: Wed, 22 May 2024 14:27:46 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 2/3] perf maps: Remove the kernel text map with
 maps__remove_maps()
To: Leo Yan <leo.yan@arm.com>, Arnaldo Carvalho de Melo <acme@kernel.org>,
 Ian Rogers <irogers@google.com>, Namhyung Kim <namhyung@kernel.org>,
 James Clark <james.clark@arm.com>,
 Athira Rajeev <atrajeev@linux.vnet.ibm.com>,
 Mark Rutland <mark.rutland@arm.com>,
 Alexander Shishkin <alexander.shishkin@linux.intel.com>,
 Jiri Olsa <jolsa@kernel.org>, linux-perf-users@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240520090647.949371-1-leo.yan@arm.com>
 <20240520090647.949371-3-leo.yan@arm.com>
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: <20240520090647.949371-3-leo.yan@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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));