Re: [PATCH V2 2/3] perf tools: Replace aggregation ID with a struct

* Re: [PATCH V2 2/3] perf tools: Replace aggregation ID with a struct
       [not found]   ` <20201028094311.8563-3-james.clark@arm.com>
@ 2020-11-12 15:18     ` John Garry
  2020-11-17 15:05       ` James Clark
  0 siblings, 1 reply; 6+ messages in thread
From: John Garry @ 2020-11-12 15:18 UTC (permalink / raw)
  To: James Clark, linux-perf-users, jolsa
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Mark Rutland, Alexander Shishkin, Namhyung Kim, Tommi Rantala,
	Christophe JAILLET, Andi Kleen, Jin Yao, Kajol Jain,
	Thomas Richter, Kan Liang, Alexey Budankov

>   
> +static void cpu_aggr_map__delete(struct cpu_aggr_map *map)
> +{
> +	if (map) {

is this check just paranoia?

> +		WARN_ONCE(refcount_read(&map->refcnt) != 0,
> +			  "cpu_aggr_map refcnt unbalanced\n");

and this?

> +		free(map);
> +	}
> +}
> +
> +static void cpu_aggr_map__put(struct cpu_aggr_map *map)
> +{
> +	if (map && refcount_dec_and_test(&map->refcnt))
> +		cpu_aggr_map__delete(map);
> +}
> +
>   static void perf_stat__exit_aggr_mode(void)

...

>   
> +struct cpu_aggr_map *cpu_aggr_map__empty_new(int nr)
> +{
> +	struct cpu_aggr_map *cpus = malloc(sizeof(*cpus) + sizeof(struct aggr_cpu_id) * nr);
> +

if (!cpus)
	return NULL

cpus->nr = nr;
...

this avoids extra indentation and {}

> +	if (cpus != NULL) {
> +		int i;
> +
> +		cpus->nr = nr;
> +		for (i = 0; i < nr; i++)
> +			cpus->map[i] = cpu_map__empty_aggr_cpu_id();
> +
> +		refcount_set(&cpus->refcnt, 1);
> +	}
> +
> +	return cpus;
> +}
> +
>   static int cpu__get_topology_int(int cpu, const char *name, int *value)
>   {
>   	char path[PATH_MAX];
> @@ -111,40 +128,47 @@ int cpu_map__get_socket_id(int cpu)
>   	return ret ?: value;
>   }
>   
> -int cpu_map__get_socket(struct perf_cpu_map *map, int idx, void *data __maybe_unused)
> +struct aggr_cpu_id cpu_map__get_socket(struct perf_cpu_map *map, int idx,
> +					void *data __maybe_unused)
>   {
>   	int cpu;
> +	struct aggr_cpu_id socket = cpu_map__empty_aggr_cpu_id();
>   
>   	if (idx > map->nr)
> -		return -1;
> +		return cpu_map__empty_aggr_cpu_id();
>   
>   	cpu = map->map[idx];
>   
> -	return cpu_map__get_socket_id(cpu);
> +	socket.id = cpu_map__get_socket_id(cpu);
> +	return socket;
>   }
>   
> -static int cmp_ids(const void *a, const void *b)
> +static int cmp_aggr_cpu_id(const void *a_pointer, const void *b_pointer)
>   {
> -	return *(int *)a - *(int *)b;
> +	struct aggr_cpu_id *a = (struct aggr_cpu_id *)a_pointer;
> +	struct aggr_cpu_id *b = (struct aggr_cpu_id *)b_pointer;
> +
> +	return a->id - b->id;
>   }
>   
> -int cpu_map__build_map(struct perf_cpu_map *cpus, struct perf_cpu_map **res,
> -		       int (*f)(struct perf_cpu_map *map, int cpu, void *data),
> +int cpu_map__build_map(struct perf_cpu_map *cpus, struct cpu_aggr_map **res,
> +		       struct aggr_cpu_id (*f)(struct perf_cpu_map *map, int cpu, void *data),
>   		       void *data)
>   {
> -	struct perf_cpu_map *c;
> +	struct cpu_aggr_map *c;
>   	int nr = cpus->nr;
> -	int cpu, s1, s2;
> +	int cpu, s2;
> +	struct aggr_cpu_id s1;
>   
>   	/* allocate as much as possible */
> -	c = calloc(1, sizeof(*c) + nr * sizeof(int));
> +	c = calloc(1, sizeof(*c) + nr * sizeof(struct aggr_cpu_id));
>   	if (!c)
>   		return -1;
>   
>   	for (cpu = 0; cpu < nr; cpu++) {
>   		s1 = f(cpus, cpu, data);
>   		for (s2 = 0; s2 < c->nr; s2++) {
> -			if (s1 == c->map[s2])
> +			if (cpu_map__compare_aggr_cpu_id(s1, c->map[s2]))
>   				break;
>   		}
>   		if (s2 == c->nr) {
> @@ -153,7 +177,7 @@ int cpu_map__build_map(struct perf_cpu_map *cpus, struct perf_cpu_map **res,
>   		}
>   	}
>   	/* ensure we process id in increasing order */
> -	qsort(c->map, c->nr, sizeof(int), cmp_ids);
> +	qsort(c->map, c->nr, sizeof(struct aggr_cpu_id), cmp_aggr_cpu_id);
>   
>   	refcount_set(&c->refcnt, 1);
>   	*res = c;
> @@ -167,23 +191,24 @@ int cpu_map__get_die_id(int cpu)
>   	return ret ?: value;
>   }
>   
> -int cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data)
> +struct aggr_cpu_id cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data)
>   {
> -	int cpu, die_id, s;
> +	int cpu, s;
> +	struct aggr_cpu_id die_id = cpu_map__empty_aggr_cpu_id();
>   
>   	if (idx > map->nr)
> -		return -1;
> +		return cpu_map__empty_aggr_cpu_id();
>   
>   	cpu = map->map[idx];
>   
> -	die_id = cpu_map__get_die_id(cpu);
> +	die_id.id = cpu_map__get_die_id(cpu);
>   	/* There is no die_id on legacy system. */
> -	if (die_id == -1)
> -		die_id = 0;
> +	if (die_id.id == -1)
> +		die_id.id = 0;
>   
> -	s = cpu_map__get_socket(map, idx, data);
> +	s = cpu_map__get_socket(map, idx, data).id;
>   	if (s == -1)
> -		return -1;
> +		return cpu_map__empty_aggr_cpu_id();
>   
>   	/*
>   	 * Encode socket in bit range 15:8
> @@ -191,13 +216,14 @@ int cpu_map__get_die(struct perf_cpu_map *map, int idx, void *data)
>   	 * we need a global id. So we combine
>   	 * socket + die id
>   	 */
> -	if (WARN_ONCE(die_id >> 8, "The die id number is too big.\n"))
> -		return -1;
> +	if (WARN_ONCE(die_id.id >> 8, "The die id number is too big.\n"))
> +		return cpu_map__empty_aggr_cpu_id();
>   
>   	if (WARN_ONCE(s >> 8, "The socket id number is too big.\n"))
> -		return -1;
> +		return cpu_map__empty_aggr_cpu_id();
>   
> -	return (s << 8) | (die_id & 0xff);
> +	die_id.id = (s << 8) | (die_id.id & 0xff);
> +	return die_id;
>   }
>   
>   int cpu_map__get_core_id(int cpu)
> @@ -211,21 +237,22 @@ int cpu_map__get_node_id(int cpu)
>   	return cpu__get_node(cpu);
>   }
>   
> -int cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data)
> +struct aggr_cpu_id cpu_map__get_core(struct perf_cpu_map *map, int idx, void *data)
>   {
> -	int cpu, s_die;
> +	int cpu;
> +	struct aggr_cpu_id core = cpu_map__empty_aggr_cpu_id();
>   
>   	if (idx > map->nr)

should pre-existing code be idx >= map->nr? I didn't check the code any 
deeper

> -		return -1;
> +		return cpu_map__empty_aggr_cpu_id();
>   
>   	cpu = map->map[idx];
>   

^ permalink raw reply	[flat|nested] 6+ messages in thread