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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Jan 2021 13:47:54 +0800
From:   "Jin, Yao" <yao.jin@...ux.intel.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     acme@...nel.org, jolsa@...nel.org, peterz@...radead.org,
        mingo@...hat.com, alexander.shishkin@...ux.intel.com,
        Linux-kernel@...r.kernel.org, ak@...ux.intel.com,
        kan.liang@...el.com, yao.jin@...el.com, ying.huang@...el.com
Subject: Re: [PATCH v7] perf stat: Fix wrong skipping for per-die aggregation

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
>