From: John Garry <john.garry@huawei.com>
To: James Clark <james.clark@arm.com>,
<linux-perf-users@vger.kernel.org>, <jolsa@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Namhyung Kim <namhyung@kernel.org>,
Tommi Rantala <tommi.t.rantala@nokia.com>,
Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
Andi Kleen <ak@linux.intel.com>,
Jin Yao <yao.jin@linux.intel.com>,
Kajol Jain <kjain@linux.ibm.com>,
Thomas Richter <tmricht@linux.ibm.com>,
Kan Liang <kan.liang@linux.intel.com>,
"Alexey Budankov" <alexey.budankov@linux.intel.com>
Subject: Re: [PATCH V2 2/3] perf tools: Replace aggregation ID with a struct
Date: Thu, 12 Nov 2020 15:18:05 +0000 [thread overview]
Message-ID: <0024879f-d326-966d-86b0-8cda91483bfe@huawei.com> (raw)
In-Reply-To: <20201028094311.8563-3-james.clark@arm.com>
>
> +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];
>
next parent reply other threads:[~2020-11-12 15:18 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20201004203545.GB217601@krava>
[not found] ` <20201028094311.8563-1-james.clark@arm.com>
[not found] ` <20201028094311.8563-3-james.clark@arm.com>
2020-11-12 15:18 ` John Garry [this message]
2020-11-17 15:05 ` [PATCH V2 2/3] perf tools: Replace aggregation ID with a struct James Clark
2020-11-17 15:48 ` James Clark
2020-11-17 15:53 ` James Clark
[not found] ` <20201028094311.8563-4-james.clark@arm.com>
[not found] ` <20201108124412.GA196289@krava>
2020-11-12 19:42 ` [PATCH V2 3/3] perf tools: fix perf stat with large socket IDs James Clark
2020-11-12 21:28 ` Jiri Olsa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0024879f-d326-966d-86b0-8cda91483bfe@huawei.com \
--to=john.garry@huawei.com \
--cc=acme@kernel.org \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=alexey.budankov@linux.intel.com \
--cc=christophe.jaillet@wanadoo.fr \
--cc=james.clark@arm.com \
--cc=jolsa@redhat.com \
--cc=kan.liang@linux.intel.com \
--cc=kjain@linux.ibm.com \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=tmricht@linux.ibm.com \
--cc=tommi.t.rantala@nokia.com \
--cc=yao.jin@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).