Re: [PATCH v3 2/3] perf: make build_cpu_topology skip offline/absent CPUs

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, Feb 17, 2017 at 12:10:25PM +0100, Jan Stancek escreveu:
> When build_cpu_topo() encounters offline/absent CPUs,
> it fails to find any sysfs entries and returns failure.
> This leads to build_cpu_topology() and write_cpu_topology()
> failing as well.
> 
> Because HEADER_CPU_TOPOLOGY has not been written, read leaves
> cpu_topology_map NULL and we get NULL ptr deref at:
> 
>  ...
>   cmd_test
>    __cmd_test
>     test_and_print
>      run_test
>       test_session_topology
>        check_cpu_topology
> 
> 36: Session topology                           :
> --- start ---

Please add two spaces before tool output, so that we can avoid breaking
scripts looking for --- as separator of patch message from patch
content.

Also look at 'git log file/you/are/modifying' to see the patch log
style, for this one I'm changing to:

perf header: Make build_cpu_topology skip offline/absent CPUs

Thanks,

- Arnaldo

> test child forked, pid 14902
> templ file: /tmp/perf-test-4CKocW
> failed to write feature HEADER_CPU_TOPOLOGY
> perf: Segmentation fault
> Obtained 9 stack frames.
> ./perf(sighandler_dump_stack+0x41) [0x5095f1]
> /lib64/libc.so.6(+0x35250) [0x7f4b7c3c9250]
> ./perf(test_session_topology+0x1db) [0x490ceb]
> ./perf() [0x475b68]
> ./perf(cmd_test+0x5b9) [0x4763c9]
> ./perf() [0x4945a3]
> ./perf(main+0x69f) [0x427e8f]
> /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f4b7c3b5b35]
> ./perf() [0x427fb9]
> test child interrupted
> ---- end ----
> Session topology: FAILED!
> 
> This patch makes build_cpu_topology() skip offline/absent CPUs,
> by checking their presence against cpu_map built from online CPUs.
> 
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  tools/perf/util/header.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> Changes in v3:
> - make ret = -1 by default, so we fail in case build_cpu_topo
>   doesn't run at all
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index d89c9c7ef4e5..ca0f12fb5fd3 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -503,24 +503,31 @@ static void free_cpu_topo(struct cpu_topo *tp)
>  
>  static struct cpu_topo *build_cpu_topology(void)
>  {
> -	struct cpu_topo *tp;
> +	struct cpu_topo *tp = NULL;
>  	void *addr;
>  	u32 nr, i;
>  	size_t sz;
>  	long ncpus;
>  	int ret = -1;
> +	struct cpu_map *map;
>  
>  	ncpus = sysconf(_SC_NPROCESSORS_CONF);
>  	if (ncpus < 0)
>  		return NULL;
>  
> +	/* build online CPU map */
> +	map = cpu_map__new(NULL);
> +	if (map == NULL) {
> +		pr_debug("failed to get system cpumap\n");
> +		return NULL;
> +	}
> +
>  	nr = (u32)(ncpus & UINT_MAX);
>  
>  	sz = nr * sizeof(char *);
> -
>  	addr = calloc(1, sizeof(*tp) + 2 * sz);
>  	if (!addr)
> -		return NULL;
> +		goto out_free;
>  
>  	tp = addr;
>  	tp->cpu_nr = nr;
> @@ -530,10 +537,16 @@ static struct cpu_topo *build_cpu_topology(void)
>  	tp->thread_siblings = addr;
>  
>  	for (i = 0; i < nr; i++) {
> +		if (!cpu_map__has(map, i))
> +			continue;
> +
>  		ret = build_cpu_topo(tp, i);
>  		if (ret < 0)
>  			break;
>  	}
> +
> +out_free:
> +	cpu_map__put(map);
>  	if (ret) {
>  		free_cpu_topo(tp);
>  		tp = NULL;
> -- 
> 1.8.3.1