Messages in this thread | | | Subject | Re: [PATCH v4] perf stat: Fix wrong skipping for per-die aggregation | From | "Jin, Yao" <> | Date | Wed, 13 Jan 2021 08:41:54 +0800 |
| |
Hi Jiri,
On 1/12/2021 6:04 PM, Jiri Olsa wrote: > On Tue, Jan 05, 2021 at 10:36:15AM +0800, Jin Yao wrote: > > SNIP > >> diff --git a/tools/perf/util/stat.c b/tools/perf/util/stat.c >> index 8ce1479c98f0..a658e0ffaf2a 100644 >> --- a/tools/perf/util/stat.c >> +++ b/tools/perf/util/stat.c >> @@ -13,6 +13,7 @@ >> #include "evlist.h" >> #include "evsel.h" >> #include "thread_map.h" >> +#include "hashmap.h" >> #include <linux/zalloc.h> >> >> void update_stats(struct stats *stats, u64 val) >> @@ -276,15 +277,29 @@ void evlist__save_aggr_prev_raw_counts(struct evlist *evlist) >> static void zero_per_pkg(struct evsel *counter) >> { >> if (counter->per_pkg_mask) >> - memset(counter->per_pkg_mask, 0, cpu__max_cpu()); >> + hashmap__clear(counter->per_pkg_mask); >> +} >> + >> +static size_t pkg_id_hash(const void *key, void *ctx __maybe_unused) >> +{ >> + int socket = (int64_t)key >> 32; >> + >> + return socket; > > could be just simple return right, why the variable? >
Sure, just simple return should be OK.
>> +} >> + >> +static bool pkg_id_equal(const void *key1, const void *key2, >> + void *ctx __maybe_unused) >> +{ >> + return (int64_t)key1 == (int64_t)key2; >> } >> >> static int check_per_pkg(struct evsel *counter, >> struct perf_counts_values *vals, int cpu, bool *skip) >> { >> - unsigned long *mask = counter->per_pkg_mask; >> + struct hashmap *mask = counter->per_pkg_mask; >> struct perf_cpu_map *cpus = evsel__cpus(counter); >> - int s; >> + int s, d, ret; >> + uint64_t key; >> >> *skip = false; >> >> @@ -295,7 +310,7 @@ static int check_per_pkg(struct evsel *counter, >> return 0; >> >> if (!mask) { >> - mask = zalloc(cpu__max_cpu()); >> + mask = hashmap__new(pkg_id_hash, pkg_id_equal, NULL); >> if (!mask) >> return -ENOMEM; >> >> @@ -317,7 +332,23 @@ static int check_per_pkg(struct evsel *counter, >> if (s < 0) >> return -1; >> >> - *skip = test_and_set_bit(s, mask) == 1; >> + /* >> + * On multi-die system, 0 < die_id < 256. On no-die system, die_id = 0. >> + * We use hashmap(socket, die) to check the used socket+die pair. >> + */ >> + d = cpu_map__get_die(cpus, cpu, NULL).die; >> + if (d < 0) >> + return -1; >> + >> + key = (uint64_t)s << 32 | (d & 0xff); > > why the masking for d? also I thought it could be bigger than 256 >
I remember in previous perf tool it would show a warning "The die id number is too big" if die_id > 256 and cpu_map__get_die() returned -1. See commit db5742b6849e ("perf stat: Support per-die aggregation").
But now the die_id checking is removed.
> would it better to do it the other way around? > > die << 32 | socket >
I think this way is OK, thanks! I will use it in v5.
> >> + if (hashmap__find(mask, (void *)key, NULL)) { >> + *skip = true; >> + } else { >> + ret = hashmap__add(mask, (void *)key, (void *)1); >> + if (ret) >> + return -1; > > you could 'return ret' at the end and skip this extra check >
Yes, define int ret = 0 and just 'return ret' at the end of function.
Thanks Jin Yao
> thanks, > jirka > >> + } >> + >> return 0; >> } >> >> -- >> 2.17.1 >> >
| |