* [PATCH v1 1/3] perf stream: Use evsel rather than evsel->idx
2024-11-14 23:07 [PATCH v1 0/3] Prefer evsel over evsel->core.idx Ian Rogers
@ 2024-11-14 23:07 ` Ian Rogers
2024-11-14 23:07 ` [PATCH v1 2/3] perf values: " Ian Rogers
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Ian Rogers @ 2024-11-14 23:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Kan Liang, Andi Kleen,
Ahelenia Ziemiańska, Chen Ni, linux-perf-users, linux-kernel
An evsel idx may not be stable due to sorting, evlist removal,
etc. Avoid use of the idx where the evsel itself can be used to avoid
these problems.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/builtin-diff.c | 4 ++--
tools/perf/util/stream.c | 7 +++----
tools/perf/util/stream.h | 10 +++++-----
3 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 82fb7773e03e..f72c791dada4 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -1019,12 +1019,12 @@ static int process_base_stream(struct data__file *data_base,
continue;
es_base = evsel_streams__entry(data_base->evlist_streams,
- evsel_base->core.idx);
+ evsel_base);
if (!es_base)
return -1;
es_pair = evsel_streams__entry(data_pair->evlist_streams,
- evsel_pair->core.idx);
+ evsel_pair);
if (!es_pair)
return -1;
diff --git a/tools/perf/util/stream.c b/tools/perf/util/stream.c
index 545e44981a27..3de4a6130853 100644
--- a/tools/perf/util/stream.c
+++ b/tools/perf/util/stream.c
@@ -52,7 +52,6 @@ static struct evlist_streams *evlist_streams__new(int nr_evsel,
goto err;
s->nr_streams_max = nr_streams_max;
- s->evsel_idx = -1;
}
els->ev_streams = es;
@@ -139,7 +138,7 @@ static int evlist__init_callchain_streams(struct evlist *evlist,
hists__output_resort(hists, NULL);
init_hot_callchain(hists, &es[i]);
- es[i].evsel_idx = pos->core.idx;
+ es[i].evsel = pos;
i++;
}
@@ -166,12 +165,12 @@ struct evlist_streams *evlist__create_streams(struct evlist *evlist,
}
struct evsel_streams *evsel_streams__entry(struct evlist_streams *els,
- int evsel_idx)
+ const struct evsel *evsel)
{
struct evsel_streams *es = els->ev_streams;
for (int i = 0; i < els->nr_evsel; i++) {
- if (es[i].evsel_idx == evsel_idx)
+ if (es[i].evsel == evsel)
return &es[i];
}
diff --git a/tools/perf/util/stream.h b/tools/perf/util/stream.h
index bee768874fea..50f7e6e04982 100644
--- a/tools/perf/util/stream.h
+++ b/tools/perf/util/stream.h
@@ -2,7 +2,9 @@
#ifndef __PERF_STREAM_H
#define __PERF_STREAM_H
-#include "callchain.h"
+struct callchain_node;
+struct evlist;
+struct evsel;
struct stream {
struct callchain_node *cnode;
@@ -11,9 +13,9 @@ struct stream {
struct evsel_streams {
struct stream *streams;
+ const struct evsel *evsel;
int nr_streams_max;
int nr_streams;
- int evsel_idx;
u64 streams_hits;
};
@@ -22,15 +24,13 @@ struct evlist_streams {
int nr_evsel;
};
-struct evlist;
-
void evlist_streams__delete(struct evlist_streams *els);
struct evlist_streams *evlist__create_streams(struct evlist *evlist,
int nr_streams_max);
struct evsel_streams *evsel_streams__entry(struct evlist_streams *els,
- int evsel_idx);
+ const struct evsel *evsel);
void evsel_streams__match(struct evsel_streams *es_base,
struct evsel_streams *es_pair);
--
2.47.0.338.g60cca15819-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v1 2/3] perf values: Use evsel rather than evsel->idx
2024-11-14 23:07 [PATCH v1 0/3] Prefer evsel over evsel->core.idx Ian Rogers
2024-11-14 23:07 ` [PATCH v1 1/3] perf stream: Use evsel rather than evsel->idx Ian Rogers
@ 2024-11-14 23:07 ` Ian Rogers
2024-11-14 23:07 ` [PATCH v1 3/3] perf annotate: Prefer passing evsel to evsel->core.idx Ian Rogers
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Ian Rogers @ 2024-11-14 23:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Kan Liang, Andi Kleen,
Ahelenia Ziemiańska, Chen Ni, linux-perf-users, linux-kernel
An evsel idx may not be stable due to sorting, evlist removal,
etc. Avoid use of the idx where the evsel itself can be used to avoid
these problems. This removed 1 values array and duplicated evsel name
strings.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/builtin-report.c | 4 +-
tools/perf/util/values.c | 106 +++++++++++++++---------------------
tools/perf/util/values.h | 9 +--
3 files changed, 51 insertions(+), 68 deletions(-)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 048c91960ba9..e5478082845c 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -348,11 +348,9 @@ static int process_read_event(const struct perf_tool *tool,
struct report *rep = container_of(tool, struct report, tool);
if (rep->show_threads) {
- const char *name = evsel__name(evsel);
int err = perf_read_values_add_value(&rep->show_threads_values,
event->read.pid, event->read.tid,
- evsel->core.idx,
- name,
+ evsel,
event->read.value);
if (err)
diff --git a/tools/perf/util/values.c b/tools/perf/util/values.c
index b9823f414f10..ec72d29f3d58 100644
--- a/tools/perf/util/values.c
+++ b/tools/perf/util/values.c
@@ -8,6 +8,7 @@
#include "values.h"
#include "debug.h"
+#include "evsel.h"
int perf_read_values_init(struct perf_read_values *values)
{
@@ -22,21 +23,17 @@ int perf_read_values_init(struct perf_read_values *values)
values->threads = 0;
values->counters_max = 16;
- values->counterrawid = malloc(values->counters_max
- * sizeof(*values->counterrawid));
- values->countername = malloc(values->counters_max
- * sizeof(*values->countername));
- if (!values->counterrawid || !values->countername) {
- pr_debug("failed to allocate read_values counters arrays");
+ values->counters = malloc(values->counters_max * sizeof(*values->counters));
+ if (!values->counters) {
+ pr_debug("failed to allocate read_values counters array");
goto out_free_counter;
}
- values->counters = 0;
+ values->num_counters = 0;
return 0;
out_free_counter:
- zfree(&values->counterrawid);
- zfree(&values->countername);
+ zfree(&values->counters);
out_free_pid:
zfree(&values->pid);
zfree(&values->tid);
@@ -56,10 +53,7 @@ void perf_read_values_destroy(struct perf_read_values *values)
zfree(&values->value);
zfree(&values->pid);
zfree(&values->tid);
- zfree(&values->counterrawid);
- for (i = 0; i < values->counters; i++)
- zfree(&values->countername[i]);
- zfree(&values->countername);
+ zfree(&values->counters);
}
static int perf_read_values__enlarge_threads(struct perf_read_values *values)
@@ -116,81 +110,71 @@ static int perf_read_values__findnew_thread(struct perf_read_values *values,
static int perf_read_values__enlarge_counters(struct perf_read_values *values)
{
- char **countername;
- int i, counters_max = values->counters_max * 2;
- u64 *counterrawid = realloc(values->counterrawid, counters_max * sizeof(*values->counterrawid));
+ int counters_max = values->counters_max * 2;
+ struct evsel **new_counters = realloc(values->counters,
+ counters_max * sizeof(*values->counters));
- if (!counterrawid) {
- pr_debug("failed to enlarge read_values rawid array");
+ if (!new_counters) {
+ pr_debug("failed to enlarge read_values counters array");
goto out_enomem;
}
- countername = realloc(values->countername, counters_max * sizeof(*values->countername));
- if (!countername) {
- pr_debug("failed to enlarge read_values rawid array");
- goto out_free_rawid;
- }
-
- for (i = 0; i < values->threads; i++) {
+ for (int i = 0; i < values->threads; i++) {
u64 *value = realloc(values->value[i], counters_max * sizeof(**values->value));
- int j;
if (!value) {
pr_debug("failed to enlarge read_values ->values array");
- goto out_free_name;
+ goto out_free_counters;
}
- for (j = values->counters_max; j < counters_max; j++)
+ for (int j = values->counters_max; j < counters_max; j++)
value[j] = 0;
values->value[i] = value;
}
values->counters_max = counters_max;
- values->counterrawid = counterrawid;
- values->countername = countername;
+ values->counters = new_counters;
return 0;
-out_free_name:
- free(countername);
-out_free_rawid:
- free(counterrawid);
+out_free_counters:
+ free(new_counters);
out_enomem:
return -ENOMEM;
}
static int perf_read_values__findnew_counter(struct perf_read_values *values,
- u64 rawid, const char *name)
+ struct evsel *evsel)
{
int i;
- for (i = 0; i < values->counters; i++)
- if (values->counterrawid[i] == rawid)
+ for (i = 0; i < values->num_counters; i++)
+ if (values->counters[i] == evsel)
return i;
- if (values->counters == values->counters_max) {
- i = perf_read_values__enlarge_counters(values);
- if (i)
- return i;
+ if (values->num_counters == values->counters_max) {
+ int err = perf_read_values__enlarge_counters(values);
+
+ if (err)
+ return err;
}
- i = values->counters++;
- values->counterrawid[i] = rawid;
- values->countername[i] = strdup(name);
+ i = values->num_counters++;
+ values->counters[i] = evsel;
return i;
}
int perf_read_values_add_value(struct perf_read_values *values,
u32 pid, u32 tid,
- u64 rawid, const char *name, u64 value)
+ struct evsel *evsel, u64 value)
{
int tindex, cindex;
tindex = perf_read_values__findnew_thread(values, pid, tid);
if (tindex < 0)
return tindex;
- cindex = perf_read_values__findnew_counter(values, rawid, name);
+ cindex = perf_read_values__findnew_counter(values, evsel);
if (cindex < 0)
return cindex;
@@ -205,15 +189,15 @@ static void perf_read_values__display_pretty(FILE *fp,
int pidwidth, tidwidth;
int *counterwidth;
- counterwidth = malloc(values->counters * sizeof(*counterwidth));
+ counterwidth = malloc(values->num_counters * sizeof(*counterwidth));
if (!counterwidth) {
fprintf(fp, "INTERNAL ERROR: Failed to allocate counterwidth array\n");
return;
}
tidwidth = 3;
pidwidth = 3;
- for (j = 0; j < values->counters; j++)
- counterwidth[j] = strlen(values->countername[j]);
+ for (j = 0; j < values->num_counters; j++)
+ counterwidth[j] = strlen(evsel__name(values->counters[j]));
for (i = 0; i < values->threads; i++) {
int width;
@@ -223,7 +207,7 @@ static void perf_read_values__display_pretty(FILE *fp,
width = snprintf(NULL, 0, "%d", values->tid[i]);
if (width > tidwidth)
tidwidth = width;
- for (j = 0; j < values->counters; j++) {
+ for (j = 0; j < values->num_counters; j++) {
width = snprintf(NULL, 0, "%" PRIu64, values->value[i][j]);
if (width > counterwidth[j])
counterwidth[j] = width;
@@ -231,14 +215,14 @@ static void perf_read_values__display_pretty(FILE *fp,
}
fprintf(fp, "# %*s %*s", pidwidth, "PID", tidwidth, "TID");
- for (j = 0; j < values->counters; j++)
- fprintf(fp, " %*s", counterwidth[j], values->countername[j]);
+ for (j = 0; j < values->num_counters; j++)
+ fprintf(fp, " %*s", counterwidth[j], evsel__name(values->counters[j]));
fprintf(fp, "\n");
for (i = 0; i < values->threads; i++) {
fprintf(fp, " %*d %*d", pidwidth, values->pid[i],
tidwidth, values->tid[i]);
- for (j = 0; j < values->counters; j++)
+ for (j = 0; j < values->num_counters; j++)
fprintf(fp, " %*" PRIu64,
counterwidth[j], values->value[i][j]);
fprintf(fp, "\n");
@@ -266,16 +250,16 @@ static void perf_read_values__display_raw(FILE *fp,
if (width > tidwidth)
tidwidth = width;
}
- for (j = 0; j < values->counters; j++) {
- width = strlen(values->countername[j]);
+ for (j = 0; j < values->num_counters; j++) {
+ width = strlen(evsel__name(values->counters[j]));
if (width > namewidth)
namewidth = width;
- width = snprintf(NULL, 0, "%" PRIx64, values->counterrawid[j]);
+ width = snprintf(NULL, 0, "%x", values->counters[j]->core.idx);
if (width > rawwidth)
rawwidth = width;
}
for (i = 0; i < values->threads; i++) {
- for (j = 0; j < values->counters; j++) {
+ for (j = 0; j < values->num_counters; j++) {
width = snprintf(NULL, 0, "%" PRIu64, values->value[i][j]);
if (width > countwidth)
countwidth = width;
@@ -287,12 +271,12 @@ static void perf_read_values__display_raw(FILE *fp,
namewidth, "Name", rawwidth, "Raw",
countwidth, "Count");
for (i = 0; i < values->threads; i++)
- for (j = 0; j < values->counters; j++)
- fprintf(fp, " %*d %*d %*s %*" PRIx64 " %*" PRIu64,
+ for (j = 0; j < values->num_counters; j++)
+ fprintf(fp, " %*d %*d %*s %*x %*" PRIu64,
pidwidth, values->pid[i],
tidwidth, values->tid[i],
- namewidth, values->countername[j],
- rawwidth, values->counterrawid[j],
+ namewidth, evsel__name(values->counters[j]),
+ rawwidth, values->counters[j]->core.idx,
countwidth, values->value[i][j]);
}
diff --git a/tools/perf/util/values.h b/tools/perf/util/values.h
index 791c1ad606c2..bbca33daca19 100644
--- a/tools/perf/util/values.h
+++ b/tools/perf/util/values.h
@@ -5,14 +5,15 @@
#include <stdio.h>
#include <linux/types.h>
+struct evsel;
+
struct perf_read_values {
int threads;
int threads_max;
u32 *pid, *tid;
- int counters;
+ int num_counters;
int counters_max;
- u64 *counterrawid;
- char **countername;
+ struct evsel **counters;
u64 **value;
};
@@ -21,7 +22,7 @@ void perf_read_values_destroy(struct perf_read_values *values);
int perf_read_values_add_value(struct perf_read_values *values,
u32 pid, u32 tid,
- u64 rawid, const char *name, u64 value);
+ struct evsel *evsel, u64 value);
void perf_read_values_display(FILE *fp, struct perf_read_values *values,
int raw);
--
2.47.0.338.g60cca15819-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH v1 3/3] perf annotate: Prefer passing evsel to evsel->core.idx
2024-11-14 23:07 [PATCH v1 0/3] Prefer evsel over evsel->core.idx Ian Rogers
2024-11-14 23:07 ` [PATCH v1 1/3] perf stream: Use evsel rather than evsel->idx Ian Rogers
2024-11-14 23:07 ` [PATCH v1 2/3] perf values: " Ian Rogers
@ 2024-11-14 23:07 ` Ian Rogers
2024-12-20 19:50 ` Arnaldo Carvalho de Melo
2024-11-18 10:18 ` [PATCH v1 0/3] Prefer evsel over evsel->core.idx James Clark
2024-12-14 0:13 ` Namhyung Kim
4 siblings, 1 reply; 9+ messages in thread
From: Ian Rogers @ 2024-11-14 23:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, Kan Liang, Andi Kleen,
Ahelenia Ziemiańska, Chen Ni, linux-perf-users, linux-kernel
An evsel idx may not be stable due to sorting, evlist removal,
etc. Try to reduce it being part of APIs by explicitly passing the
evsel in annotate code. Internally the code just reads evsel->core.idx
so behavior is unchanged.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/builtin-top.c | 4 ++--
tools/perf/ui/browsers/annotate.c | 2 +-
tools/perf/util/annotate.c | 32 +++++++++++++++----------------
tools/perf/util/annotate.h | 20 ++++++++++---------
4 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 724a79386321..881e6cf26979 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -267,9 +267,9 @@ static void perf_top__show_details(struct perf_top *top)
if (top->evlist->enabled) {
if (top->zero)
- symbol__annotate_zero_histogram(symbol, top->sym_evsel->core.idx);
+ symbol__annotate_zero_histogram(symbol, top->sym_evsel);
else
- symbol__annotate_decay_histogram(symbol, top->sym_evsel->core.idx);
+ symbol__annotate_decay_histogram(symbol, top->sym_evsel);
}
if (more != 0)
printf("%d lines not displayed, maybe increase display entries [e]\n", more);
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index d7e727345dab..135d6ce88fb3 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -754,7 +754,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
hbt->timer(hbt->arg);
if (delay_secs != 0) {
- symbol__annotate_decay_histogram(sym, evsel->core.idx);
+ symbol__annotate_decay_histogram(sym, evsel);
hists__scnprintf_title(hists, title, sizeof(title));
annotate_browser__show(&browser->b, title, help);
}
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index b1d98da79be8..9bd3f472a525 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -209,7 +209,7 @@ static int __symbol__account_cycles(struct cyc_hist *ch,
}
static int __symbol__inc_addr_samples(struct map_symbol *ms,
- struct annotated_source *src, int evidx, u64 addr,
+ struct annotated_source *src, struct evsel *evsel, u64 addr,
struct perf_sample *sample)
{
struct symbol *sym = ms->sym;
@@ -228,14 +228,14 @@ static int __symbol__inc_addr_samples(struct map_symbol *ms,
}
offset = addr - sym->start;
- h = annotated_source__histogram(src, evidx);
+ h = annotated_source__histogram(src, evsel);
if (h == NULL) {
pr_debug("%s(%d): ENOMEM! sym->name=%s, start=%#" PRIx64 ", addr=%#" PRIx64 ", end=%#" PRIx64 ", func: %d\n",
__func__, __LINE__, sym->name, sym->start, addr, sym->end, sym->type == STT_FUNC);
return -ENOMEM;
}
- hash_key = offset << 16 | evidx;
+ hash_key = offset << 16 | evsel->core.idx;
if (!hashmap__find(src->samples, hash_key, &entry)) {
entry = zalloc(sizeof(*entry));
if (entry == NULL)
@@ -252,7 +252,7 @@ static int __symbol__inc_addr_samples(struct map_symbol *ms,
pr_debug3("%#" PRIx64 " %s: period++ [addr: %#" PRIx64 ", %#" PRIx64
", evidx=%d] => nr_samples: %" PRIu64 ", period: %" PRIu64 "\n",
- sym->start, sym->name, addr, addr - sym->start, evidx,
+ sym->start, sym->name, addr, addr - sym->start, evsel->core.idx,
entry->nr_samples, entry->period);
return 0;
}
@@ -323,7 +323,7 @@ static int symbol__inc_addr_samples(struct map_symbol *ms,
if (sym == NULL)
return 0;
src = symbol__hists(sym, evsel->evlist->core.nr_entries);
- return src ? __symbol__inc_addr_samples(ms, src, evsel->core.idx, addr, sample) : 0;
+ return src ? __symbol__inc_addr_samples(ms, src, evsel, addr, sample) : 0;
}
static int symbol__account_br_cntr(struct annotated_branch *branch,
@@ -861,15 +861,14 @@ static void calc_percent(struct annotation *notes,
s64 offset, s64 end)
{
struct hists *hists = evsel__hists(evsel);
- int evidx = evsel->core.idx;
- struct sym_hist *sym_hist = annotation__histogram(notes, evidx);
+ struct sym_hist *sym_hist = annotation__histogram(notes, evsel);
unsigned int hits = 0;
u64 period = 0;
while (offset < end) {
struct sym_hist_entry *entry;
- entry = annotated_source__hist_entry(notes->src, evidx, offset);
+ entry = annotated_source__hist_entry(notes->src, evsel, offset);
if (entry) {
hits += entry->nr_samples;
period += entry->period;
@@ -1140,15 +1139,14 @@ static void print_summary(struct rb_root *root, const char *filename)
static void symbol__annotate_hits(struct symbol *sym, struct evsel *evsel)
{
- int evidx = evsel->core.idx;
struct annotation *notes = symbol__annotation(sym);
- struct sym_hist *h = annotation__histogram(notes, evidx);
+ struct sym_hist *h = annotation__histogram(notes, evsel);
u64 len = symbol__size(sym), offset;
for (offset = 0; offset < len; ++offset) {
struct sym_hist_entry *entry;
- entry = annotated_source__hist_entry(notes->src, evidx, offset);
+ entry = annotated_source__hist_entry(notes->src, evsel, offset);
if (entry && entry->nr_samples != 0)
printf("%*" PRIx64 ": %" PRIu64 "\n", BITS_PER_LONG / 2,
sym->start + offset, entry->nr_samples);
@@ -1178,7 +1176,7 @@ int symbol__annotate_printf(struct map_symbol *ms, struct evsel *evsel)
const char *d_filename;
const char *evsel_name = evsel__name(evsel);
struct annotation *notes = symbol__annotation(sym);
- struct sym_hist *h = annotation__histogram(notes, evsel->core.idx);
+ struct sym_hist *h = annotation__histogram(notes, evsel);
struct annotation_line *pos, *queue = NULL;
struct annotation_options *opts = &annotate_opts;
u64 start = map__rip_2objdump(map, sym->start);
@@ -1364,18 +1362,18 @@ int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel)
return err;
}
-void symbol__annotate_zero_histogram(struct symbol *sym, int evidx)
+void symbol__annotate_zero_histogram(struct symbol *sym, struct evsel *evsel)
{
struct annotation *notes = symbol__annotation(sym);
- struct sym_hist *h = annotation__histogram(notes, evidx);
+ struct sym_hist *h = annotation__histogram(notes, evsel);
memset(h, 0, sizeof(*notes->src->histograms) * notes->src->nr_histograms);
}
-void symbol__annotate_decay_histogram(struct symbol *sym, int evidx)
+void symbol__annotate_decay_histogram(struct symbol *sym, struct evsel *evsel)
{
struct annotation *notes = symbol__annotation(sym);
- struct sym_hist *h = annotation__histogram(notes, evidx);
+ struct sym_hist *h = annotation__histogram(notes, evsel);
struct annotation_line *al;
h->nr_samples = 0;
@@ -1385,7 +1383,7 @@ void symbol__annotate_decay_histogram(struct symbol *sym, int evidx)
if (al->offset == -1)
continue;
- entry = annotated_source__hist_entry(notes->src, evidx, al->offset);
+ entry = annotated_source__hist_entry(notes->src, evsel, al->offset);
if (entry == NULL)
continue;
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 8b9e05a1932f..2458e99abc18 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -15,6 +15,7 @@
#include "hashmap.h"
#include "disasm.h"
#include "branch.h"
+#include "evsel.h"
struct hist_browser_timer;
struct hist_entry;
@@ -23,7 +24,6 @@ struct map_symbol;
struct addr_map_symbol;
struct option;
struct perf_sample;
-struct evsel;
struct symbol;
struct annotated_data_type;
@@ -367,21 +367,23 @@ static inline u8 annotation__br_cntr_width(void)
void annotation__update_column_widths(struct annotation *notes);
void annotation__toggle_full_addr(struct annotation *notes, struct map_symbol *ms);
-static inline struct sym_hist *annotated_source__histogram(struct annotated_source *src, int idx)
+static inline struct sym_hist *annotated_source__histogram(struct annotated_source *src,
+ const struct evsel *evsel)
{
- return &src->histograms[idx];
+ return &src->histograms[evsel->core.idx];
}
-static inline struct sym_hist *annotation__histogram(struct annotation *notes, int idx)
+static inline struct sym_hist *annotation__histogram(struct annotation *notes,
+ const struct evsel *evsel)
{
- return annotated_source__histogram(notes->src, idx);
+ return annotated_source__histogram(notes->src, evsel);
}
static inline struct sym_hist_entry *
-annotated_source__hist_entry(struct annotated_source *src, int idx, u64 offset)
+annotated_source__hist_entry(struct annotated_source *src, const struct evsel *evsel, u64 offset)
{
struct sym_hist_entry *entry;
- long key = offset << 16 | idx;
+ long key = offset << 16 | evsel->core.idx;
if (!hashmap__find(src->samples, key, &entry))
return NULL;
@@ -442,8 +444,8 @@ enum symbol_disassemble_errno {
int symbol__strerror_disassemble(struct map_symbol *ms, int errnum, char *buf, size_t buflen);
int symbol__annotate_printf(struct map_symbol *ms, struct evsel *evsel);
-void symbol__annotate_zero_histogram(struct symbol *sym, int evidx);
-void symbol__annotate_decay_histogram(struct symbol *sym, int evidx);
+void symbol__annotate_zero_histogram(struct symbol *sym, struct evsel *evsel);
+void symbol__annotate_decay_histogram(struct symbol *sym, struct evsel *evsel);
void annotated_source__purge(struct annotated_source *as);
int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel);
--
2.47.0.338.g60cca15819-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1 3/3] perf annotate: Prefer passing evsel to evsel->core.idx
2024-11-14 23:07 ` [PATCH v1 3/3] perf annotate: Prefer passing evsel to evsel->core.idx Ian Rogers
@ 2024-12-20 19:50 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-12-20 19:50 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang,
Andi Kleen, Ahelenia Ziemiańska, Chen Ni, linux-perf-users,
linux-kernel
On Thu, Nov 14, 2024 at 03:07:13PM -0800, Ian Rogers wrote:
> An evsel idx may not be stable due to sorting, evlist removal,
> etc. Try to reduce it being part of APIs by explicitly passing the
> evsel in annotate code. Internally the code just reads evsel->core.idx
> so behavior is unchanged.
I think this is where this comes into play, are you running 'make -C
tools/perf tools-build' before submitting?
I'll have to stop now, but will fix this if you don´t have time for a
v2.
- Arnaldo
ui/gtk/annotate.c: In function ‘perf_gtk__get_percent’:
ui/gtk/annotate.c:45:48: error: passing argument 2 of ‘annotation__histogram’ makes pointer from integer without a cast [-Wint-conversion]
45 | symhist = annotation__histogram(notes, evidx);
| ^~~~~
| |
| int
In file included from ui/gtk/annotate.c:5:
/home/acme/git/perf-tools-next/tools/perf/util/annotate.h:383:74: note: expected ‘const struct evsel *’ but argument is of type ‘int’
383 | const struct evsel *evsel)
| ~~~~~~~~~~~~~~~~~~~~^~~~~
ui/gtk/annotate.c:46:58: error: passing argument 2 of ‘annotated_source__hist_entry’ makes pointer from integer without a cast [-Wint-conversion]
46 | entry = annotated_source__hist_entry(notes->src, evidx, dl->al.offset);
| ^~~~~
| |
| int
/home/acme/git/perf-tools-next/tools/perf/util/annotate.h:389:80: note: expected ‘const struct evsel *’ but argument is of type ‘int’
389 | annotated_source__hist_entry(struct annotated_source *src, const struct evsel *evsel, u64 offset)
| ~~~~~~~~~~~~~~~~~~~~^~~~~
TEST tests/shell/test_task_analyzer.sh.shellcheck_log
make[6]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:105: ui/gtk/annotate.o] Error 1
TEST tests/shell/test_uprobe_from_different_cu.sh.shellcheck_log
CC util/session.o
make[5]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:158: ui/gtk] Error 2
make[4]: *** [Makefile.perf:802: gtk-in.o] Error 2
make[4]: *** Waiting for unfinished jobs....
CC util/tool.o
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 0/3] Prefer evsel over evsel->core.idx
2024-11-14 23:07 [PATCH v1 0/3] Prefer evsel over evsel->core.idx Ian Rogers
` (2 preceding siblings ...)
2024-11-14 23:07 ` [PATCH v1 3/3] perf annotate: Prefer passing evsel to evsel->core.idx Ian Rogers
@ 2024-11-18 10:18 ` James Clark
2024-12-14 0:13 ` Namhyung Kim
4 siblings, 0 replies; 9+ messages in thread
From: James Clark @ 2024-11-18 10:18 UTC (permalink / raw)
To: Ian Rogers, Linux perf Profiling
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Adrian Hunter, Kan Liang, Andi Kleen, Ahelenia Ziemiańska,
Chen Ni, linux-kernel
On 14/11/2024 11:07 pm, Ian Rogers wrote:
> James Clark's patches fixing evsel->core.idx [1] reminded me that we
> pass the int value around unnecessarily. Passing the evsel avoids
> issues if the evlist is reordered but paired with sanitizers we can
> also know when something is used when it shouldn't be. These patches
> do some initial work reducing the use of evsel->core.idx or reducing
> the API to pass evsels and not their interior index.
>
> [1] https://lore.kernel.org/lkml/20241114160450.295844-2-james.clark@linaro.org/
>
> Ian Rogers (3):
> perf stream: Use evsel rather than evsel->idx
> perf values: Use evsel rather than evsel->idx
> perf annotate: Prefer passing evsel to evsel->core.idx
>
> tools/perf/builtin-diff.c | 4 +-
> tools/perf/builtin-report.c | 4 +-
> tools/perf/builtin-top.c | 4 +-
> tools/perf/ui/browsers/annotate.c | 2 +-
> tools/perf/util/annotate.c | 32 +++++----
> tools/perf/util/annotate.h | 20 +++---
> tools/perf/util/stream.c | 7 +-
> tools/perf/util/stream.h | 10 +--
> tools/perf/util/values.c | 106 +++++++++++++-----------------
> tools/perf/util/values.h | 9 +--
> 10 files changed, 90 insertions(+), 108 deletions(-)
>
Reviewed-by: James Clark <james.clark@linaro.org>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1 0/3] Prefer evsel over evsel->core.idx
2024-11-14 23:07 [PATCH v1 0/3] Prefer evsel over evsel->core.idx Ian Rogers
` (3 preceding siblings ...)
2024-11-18 10:18 ` [PATCH v1 0/3] Prefer evsel over evsel->core.idx James Clark
@ 2024-12-14 0:13 ` Namhyung Kim
2024-12-20 18:57 ` Ian Rogers
4 siblings, 1 reply; 9+ messages in thread
From: Namhyung Kim @ 2024-12-14 0:13 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Kan Liang, Andi Kleen, Ahelenia Ziemiańska, Chen Ni,
linux-perf-users, linux-kernel
On Thu, Nov 14, 2024 at 03:07:10PM -0800, Ian Rogers wrote:
> James Clark's patches fixing evsel->core.idx [1] reminded me that we
> pass the int value around unnecessarily. Passing the evsel avoids
> issues if the evlist is reordered but paired with sanitizers we can
> also know when something is used when it shouldn't be. These patches
> do some initial work reducing the use of evsel->core.idx or reducing
> the API to pass evsels and not their interior index.
>
> [1] https://lore.kernel.org/lkml/20241114160450.295844-2-james.clark@linaro.org/
>
> Ian Rogers (3):
> perf stream: Use evsel rather than evsel->idx
> perf values: Use evsel rather than evsel->idx
> perf annotate: Prefer passing evsel to evsel->core.idx
Reviewed-by: Namhyung Kim <namhyung@kernel.org>
Thanks,
Namhyung
>
> tools/perf/builtin-diff.c | 4 +-
> tools/perf/builtin-report.c | 4 +-
> tools/perf/builtin-top.c | 4 +-
> tools/perf/ui/browsers/annotate.c | 2 +-
> tools/perf/util/annotate.c | 32 +++++----
> tools/perf/util/annotate.h | 20 +++---
> tools/perf/util/stream.c | 7 +-
> tools/perf/util/stream.h | 10 +--
> tools/perf/util/values.c | 106 +++++++++++++-----------------
> tools/perf/util/values.h | 9 +--
> 10 files changed, 90 insertions(+), 108 deletions(-)
>
> --
> 2.47.0.338.g60cca15819-goog
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1 0/3] Prefer evsel over evsel->core.idx
2024-12-14 0:13 ` Namhyung Kim
@ 2024-12-20 18:57 ` Ian Rogers
2024-12-20 19:45 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 9+ messages in thread
From: Ian Rogers @ 2024-12-20 18:57 UTC (permalink / raw)
To: Namhyung Kim
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Kan Liang, Andi Kleen, Ahelenia Ziemiańska, Chen Ni,
linux-perf-users, linux-kernel
On Fri, Dec 13, 2024 at 4:13 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Nov 14, 2024 at 03:07:10PM -0800, Ian Rogers wrote:
> > James Clark's patches fixing evsel->core.idx [1] reminded me that we
> > pass the int value around unnecessarily. Passing the evsel avoids
> > issues if the evlist is reordered but paired with sanitizers we can
> > also know when something is used when it shouldn't be. These patches
> > do some initial work reducing the use of evsel->core.idx or reducing
> > the API to pass evsels and not their interior index.
> >
> > [1] https://lore.kernel.org/lkml/20241114160450.295844-2-james.clark@linaro.org/
> >
> > Ian Rogers (3):
> > perf stream: Use evsel rather than evsel->idx
> > perf values: Use evsel rather than evsel->idx
> > perf annotate: Prefer passing evsel to evsel->core.idx
Ping.
Reviewed-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: James Clark <james.clark@linaro.org>
Thanks,
Ian
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 0/3] Prefer evsel over evsel->core.idx
2024-12-20 18:57 ` Ian Rogers
@ 2024-12-20 19:45 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2024-12-20 19:45 UTC (permalink / raw)
To: Ian Rogers
Cc: Namhyung Kim, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang,
Andi Kleen, Ahelenia Ziemiańska, Chen Ni, linux-perf-users,
linux-kernel
On Fri, Dec 20, 2024 at 10:57:58AM -0800, Ian Rogers wrote:
> On Fri, Dec 13, 2024 at 4:13 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Thu, Nov 14, 2024 at 03:07:10PM -0800, Ian Rogers wrote:
> > > James Clark's patches fixing evsel->core.idx [1] reminded me that we
> > > pass the int value around unnecessarily. Passing the evsel avoids
> > > issues if the evlist is reordered but paired with sanitizers we can
> > > also know when something is used when it shouldn't be. These patches
> > > do some initial work reducing the use of evsel->core.idx or reducing
> > > the API to pass evsels and not their interior index.
> > >
> > > [1] https://lore.kernel.org/lkml/20241114160450.295844-2-james.clark@linaro.org/
> > >
> > > Ian Rogers (3):
> > > perf stream: Use evsel rather than evsel->idx
> > > perf values: Use evsel rather than evsel->idx
> > > perf annotate: Prefer passing evsel to evsel->core.idx
>
> Ping.
>
> Reviewed-by: Namhyung Kim <namhyung@kernel.org>
> Reviewed-by: James Clark <james.clark@linaro.org>
Thanks, applied to perf-tools-next,
- Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread