Re: [PATCH v7] perf stat: Fix wrong skipping for per-die aggregation

["Jin, Yao" <yao.jin@linux.intel.com>]

Hi Jiri,

On 1/24/2021 6:57 AM, Jiri Olsa wrote:
> On Thu, Jan 21, 2021 at 12:21:36PM +0800, Jin, Yao wrote:
> 
> sNIP
> 
>> mask = hashmap__new(pkg_id_hash, pkg_id_equal, NULL);
>> d = cpu_map__get_die(cpus, cpu, NULL).die;
>> key = (size_t)d << KEY_SHIFT | s;	/* s is socket id */
>> if (hashmap__find(mask, (void *)key, NULL))
>> 	*skip = true;
>> else
>> 	ret = hashmap__add(mask, (void *)key, (void *)1);
>>
>> If we use 'unsigned long' to replace 'size_t', it reports the build error for 32 bits:
>>
>> stat.c:320:23: warning: passing argument 1 of ‘hashmap__new’ from
>> incompatible pointer type [-Wincompatible-pointer-types]
>>     mask = hashmap__new(pkg_id_hash, pkg_id_equal, NULL);
>>                         ^~~~~~~~~~~
>> In file included from stat.c:16:
>> hashmap.h:75:17: note: expected ‘hashmap_hash_fn’ {aka ‘unsigned int
>> (*)(const void *, void *)’} but argument is of type ‘long unsigned int
>> (*)(const void *, void *)’
>>
>> If we use "unsigned int", it's not good for 64 bits. So I still use 'size_t' in this patch.
>>
>> Any comments for this idea (using conditional compilation)?
> 
> isn't it simpler to allocate the key then? like below
> (just compile tested)
> 
> jirka
> 

Hmm, Each method has advantages and disadvantages.

My method uses conditional compilation but it looks a bit complicated. The advantage is it doesn't 
need to allocate the memory for key.

If you need me to post v8, I'd love to.

Anyway, either method is fine for me. :)

> 
> ---
> diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c
> index 5aba8fa92386..195fda142c98 100644
> --- a/tools/perf/util/stat.c
> +++ b/tools/perf/util/stat.c
> @@ -276,19 +276,31 @@ void evlist__save_aggr_prev_raw_counts(struct evlist *evlist)
>   
>   static void zero_per_pkg(struct evsel *counter)
>   {
> -	if (counter->per_pkg_mask)
> +	struct hashmap_entry *cur;
> +	size_t bkt;
> +
> +	if (counter->per_pkg_mask) {
> +		hashmap__for_each_entry(counter->per_pkg_mask, cur, bkt)
> +			free((char *)cur->key);
> +
>   		hashmap__clear(counter->per_pkg_mask);
> +	}
>   }
>   
> -static size_t pkg_id_hash(const void *key, void *ctx __maybe_unused)
> +static size_t pkg_id_hash(const void *__key, void *ctx __maybe_unused)
>   {
> -	return (size_t)key & 0xffff;
> +	uint64_t *key = (uint64_t*) __key;
> +
> +	return *key & 0xffffffff;
>   }
>   
> -static bool pkg_id_equal(const void *key1, const void *key2,
> +static bool pkg_id_equal(const void *__key1, const void *__key2,
>   			 void *ctx __maybe_unused)
>   {
> -	return (size_t)key1 == (size_t)key2;
> +	uint64_t *key1 = (uint64_t*) __key1;
> +	uint64_t *key2 = (uint64_t*) __key2;
> +
> +	return *key1 == *key2;
>   }
>   
>   static int check_per_pkg(struct evsel *counter,
> @@ -297,7 +309,7 @@ static int check_per_pkg(struct evsel *counter,
>   	struct hashmap *mask = counter->per_pkg_mask;
>   	struct perf_cpu_map *cpus = evsel__cpus(counter);
>   	int s, d, ret = 0;
> -	size_t key;
> +	uint64_t *key;
>   
>   	*skip = false;
>   
> @@ -338,7 +350,11 @@ static int check_per_pkg(struct evsel *counter,
>   	if (d < 0)
>   		return -1;
>   
> -	key = (size_t)d << 16 | s;
> +	key = malloc(sizeof(*key));
> +	if (!key)
> +		return -ENOMEM;
> +
> +	*key = (size_t)d << 32 | s;

Should be "*key = (uint64_t)d << 32 | s;"?

Otherwise on 32 bits system, a build warning is:

stat.c: In function ‘check_per_pkg’:
stat.c:357:19: warning: left shift count >= width of type [-Wshift-count-overflow]
   *key = (size_t)d << 32 | s;

Thanks
Jin Yao

>   	if (hashmap__find(mask, (void *)key, NULL))
>   		*skip = true;
>   	else
>